On Jul 24, 2013, at 10:09 AM, Yongming Zhao <ming....@gmail.com> wrote:
> great feedback, we'd discuss it and update how we learned from the last > several days. > > we will try our best to get this in v3.4. FWIW, I'd support backporting this change to a stable 3.4.x release provided there was backwards compatibility with existing configurations. > > > thanks > > 在 2013-7-25，上午1:01，Leif Hedstrom <zw...@apache.org> 写道： > >> On 7/22/13 12:43 AM, Yongming Zhao wrote: >>> I'd like to include TS-1967, this is a better throttling than the net >>> throttling, which will not work well in most of the situation, it may case >>> either OOM or killed by heart beat from cop. we will provide another patch >>> for TS-1967, will fix the bug James point out. >>> >>> then we can remove the net throttling, due to this s feature change, if we >>> don't include it in v3.3.5, we'd delay to the V3.5-3.6 circle. >> >> So i generally agree with splitting out the connection throttling in to >> "client" (_in) vs "server" (_out) limits and counters. What I dislike with >> this patch is that it feels incomplete. I'm also very concerned that this is >> a significant change, at a very late stage. I'm very torn on it, delaying >> this change until after v3.4.0 means keeping the configuration mess for >> another ~1 year to stay compatible within the major relase. >> >> In any case, IF we decide to incorporate this into v3.3.5, I'd like to see >> the following changes to the patch: >> >> 1. A patch that applies cleanly to current master. >> 2. It needs to either add another configuration, which is the number of >> file descriptors we need to use, or, it needs to calculate this based on >> something other than proxy.config.net.connections_throttle. >> 3. I'd like for proxy.config.net.connections_throttle to go away entirely. >> 4. It looks like *maybe* fds_accept was intended to replace / solve issue >> 2) and 3) above, but as far as I can tell, it's never used. >> 5. I'd like to have consistent namings of the configuration options. I find >> the two config names, and internal variables, very confusing. >> 6. I'd like to get a better understanding how this correlates to e.g. >> proxy.config.http.server_max_connections. For example, should the max >> FDs be the sum of max-client connections and >> proxy.config.http.server_max_connections ? (I think it might be, at >> least as a default). What gets complicated is that we also have >> proxy.config.http.origin_max_connections, which can yield basically >> unlimited number of connections unless throttled otherwise. E.g. should >> the sum of all origin_max_connections (which is per origin) always be >> less than proxy.config.http.server_max_connections as an invariant? >> >> >> As for 4), what's really confusing is that the patch actually has two >> features: the first is to limit the number of concurrently accepted >> connections (this is what the patch implies). The second is a throttle on >> the max number of active connections, the difference being that active >> connections have an HttpSM that's doing stuff. >> >> A suggestion for better variable names, which correlate better with existing >> "server" configs, would be >> >> proxy.config.net.client_max_connections >> proxy.config.net.client_max_active_connections >> >> >> Alternatively, we could rename them all to stick to the _in and _out >> prefixes, to avoid the confusion of what "server" refers to (we randomly >> choose "server" to mean the proxy server or the origin server in our >> configs, thanks James for making me look, I'm now blind). E.g. >> >> proxy.config.net.max_connections_in >> proxy.config.net.max_active_connections_in >> proxy.config.net.max_connections_out >> proxy.config.net.max_active_connections_out (this is new, no such feature >> today) >> proxy.config.net.max_per_origin_connections_out (replaces >> proxy.config.http.origin_max_connections) >> >> >> Looking at it, I definitely like this last format the best. It's clear, >> consistent with other configs, and more important, is completely >> unambiguous. We would with this config setup the number of FDs with a >> default of (proxy.config.net.max_connections_in + >> proxy.config.net.max_connections_out + padding), but it still needs an >> overridable configuration to deal with the case of someone using >> proxy.config.net.max_per_origin_connections_out. So, perhaps also a new >> config >> >> proxy.config.net.file_descriptors >> >> >> I'm definitely pro these changes, but I think they need some work for sure. >> >> Thoughts? >> >> -- Leif >> >
