On 26/04/2014 9:33 p.m., Tsantilas Christos wrote: > Hi all. > This patch implements "standby=N" option for cache_peer configuration > parameter. > > This feature already discussed under the "steady peer connection pool" > and "[PREVIEW] cache_peer standby=N" squid-dev mail threads. This patch > support SSL peers. > > A detailed description of the patch exist in patch preamble. > > This is a Measurement Factory project. >
+1 since it looks okay code-wise. The issues below which need fixing are all documentatino and can be fixed without another review. in src/cache_cf.cc: * please use "ERROR:" instead of "parse_peer:" in 'fatalf("parse_peer: '... in cf.data.pre: * the new documentation for cache_peer max-conn= now states that idle connections count against teh limit then goes on to: " A peer exceeding the limit is not used for new requests unless a standby connection is available. " - this seems wrong as it states that new requests will not be sent even if idle connections are available but no standby connections, or standby not in use at all. in src/pconn.cc: * Please document PconnPool::closeN() limitations: The closeN() functionality is to prevent a *single* pooled destination overloading with connections. "Being fair" has nothing to do with it. For example when connections to 1.2.3.4:example.com overload, it makes no sense dropping idle 10.0.2.34:foo.local connections. - The new closeN() API makes all servers with connections in the pool pay the cost of a single destination IP+domain overload. This only works for dedicated pools such as standby and ICAP where the entire pool belongs to a single peer server with one piece of software receiving all connections regardless of IP. Please fix the above issues before commit. There is also a design issue in regards to DNS. The existing pools all suffer from this, so I am bringing this up as an FYI rather than a fix request. When a peer is resolved for a second time there appears to be no filter over whether the existing standby connections are still valid (going to the same destination IPs). It seems to me we should close standby connections to unused IPs and begin opening new ones to any new IPs found. In the case of standby pools where the peer is under control of the one admin I would expect that a change of IP means the peer has actually abandoned the old IP(s). For the general idle pool(s) and ICAP service pools this is far less certain so we have been getting away with it. Amos