+1 I think we should remove the code to be send these configuration options in a cluster request. Reason for this are: 1. These options are already propagated in cluster config 2. Cleaner code, able to remove code and simplify the cluster request 3. Seems odd to me to pass around a subset of the configuration to clusters per request and isn't consistent with other configuration options
-Bryan On Nov 3, 2013, at 12:27 PM, Leif Hedstrom <zw...@apache.org> wrote: > 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 >
