Re: [PATCH] cache_peer standby=N

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
Received on Sun Apr 27 2014 - 12:10:24 MDT