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. 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 >
