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
