Hi all,
I’d like to get some closure on the TS-1919 Jira issue. There have been
concerns raised about this patch, and I will try to explain the pros and the
cons of this patch. Note that this is committed on the v5.0.x branch (it’s an
incompatible change), and I really want to decide the future of this commit
such that we can either agree to keep it, or back it out right now.
Explanation of the patch.
TS-1919 eliminates a helper class named CacheLookupHttpConfig. This is used for
the Cache Clustering feature to transport 4 configurations between the cluster
members, on every response. This is to assure that both members of a negotiated
transaction use the same value for those 4 configurations. The configurations
passed along are:
proxy.config.http.cache.enable_default_vary_headers
proxy.config.http.cache.vary_default_text
proxy.config.http.cache.vary_default_images
proxy.config.http.cache.vary_default_other
Instead of passing along the CacheLookupHttpConfig class while processing a
request, we now simply pass along the normal HTTP configuration
(HttpConfigParams); this holds all configurations, including the overridable
configurations.
In addition to this, the helper class also encapsulated 5 additional
configurations which are necessary by the cache code, but the changes in
TS-1919 doesn’t affect this; those configs are still passed along, as will be
explained further. However, *only* the four configs above are transferred as
part of the clustering messages. In fact, TS-1919 would make it possible to
make those 5 configurations overridable. In any case, these additional configs
are
proxy.config.http.global_user_agent_header
proxy.config.http.cache.ignore_accept_mismatch
proxy.config.http.cache.ignore_accept_language_mismatch
proxy.config.http.cache.ignore_accept_encoding_mismatch
proxy.config.http.cache.ignore_accept_charset_mismatch
Why I thought this was a useful patch?
There are a number of reasons why I thought this was necessary to change:
It much more cleanly separates the HTTP SM from the cache core. For example,
this allows us to eliminate the ifdef’s around HTTP_CACHE. The patch uses
opaque void*’s when passing module boundaries.
Most importantly, it allows us to pass along the full, overridable Http txn
config. This means that some (but definitely not all) cache specific
records.config parameters can now be made overridable per remap rule or per
transaction via APIs. This is a fairly frequently requested RFE.
It simplifies the cluster protocol, and reduces overhead around allocations,
copying, marshaling and unmarshalling the 4 configs above, as well as the other
5 configs that it has to copy as part of the cache dependency.
What are the downsides and concerns? Well, the one that was brought up was that
being able to pass along these 4 configurations across the clustering
communications was mission critical. I can’t speak for that view, since I don’t
have that use case. The other major downside is that this breaks compatibility,
such that you can not mix a v4.x box with a 5.x box in a cache cluster (they
will split).
I feel that cluster configurations (broadcasted) solves most of the concerns in
that if you change e.g. Vary: default_text, it’ll get distributed within a few
seconds. Also, I feel that neither of the 4 settings are something you would
change particularly frequently, so in a very worst case scenario, you’d take
the cluster out of rotation while pushing that config change to all members.
Please discuss and voice your thoughts here. I’d really like to get a closure
on this, such that I can start implementing per transaction overridable cache
configurations for v5.0.x if we decide to keep TS-1919 as it is. In particular,
if you have a strong “-1” opinion on this commit as it is, please speak up now.
Cheers,
— Leif
