Re: [PATCH] cache_peer standby=N

On 04/27/2014 06:09 AM, Amos Jeffries wrote:
> in cf.data.pre:
> * the new documentation for cache_peer max-conn= now states that idle
> connections count against teh limit

Yes, they do. The patch did not change that.

> then goes on to:
> "
> A peer exceeding the limit is not used for new
> requests unless a standby connection is available.
> "

The above is also true. The patch did not change the old max-conn
behavior [in the absence of the now-supported standby connections].

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

Not sure what you mean by "this", but the updated documentation appears
to be correct. The [now documented] max-conn behavior is wrong. That old
max-conn bug is mentioned in the proposed commit message:

> Fixed max-conn documentation and XXXed a peerHTTPOkay() bug (now in
> peerHasConnAvailable()) that results in max-conn limit preventing the use of a
> peer with idle persistent connections.

Fixing idle connection handling by max-conn is outside this patch scope
and is not trivial as the added XXX now documents:

> +bool
> +peerHasConnAvailable(const CachePeer *p)
> +{
> + // Standby connections can be used without opening new connections.
> + const int standbys = p->standby.pool ? p->standby.pool->count() : 0;
> +
> + // XXX: Some idle pconns can be used without opening new connections.
> + // Complication: Idle pconns cannot be reused for some requests.
> + const int usableIdles = 0;
> +
> + const int available = standbys + usableIdles;

> in src/pconn.cc:
> * Please document PconnPool::closeN() limitations:

I believe the changed closeN() is documented correctly:

> + void closeN(int n); ///< closes any n connections

> The closeN() functionality is to prevent a *single* pooled destination
> overloading with connections.

The previously unused "single destination" semantics is no longer
supported. The updated closeN() just closes N connections. This is not a
limitation of the current interface, it is its definition.

> "Being fair" has nothing to do with it.

With the new closeN(), there is a notion of fairness because the method
can pick which connections to close. Whenever there is a freedom of
choice, the algorithm "fairness" is a consideration.

[Technically, even the old method had some freedom of choice, but that
does not matter anymore.]

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

Indeed. If/when we start supporting per-destination limits, we will need
to add a different method such as closeNto().

> - The new closeN() API makes all servers with connections in the pool
> pay the cost of a single destination IP+domain overload.

closeN() does not know and should not know why N connections need to be
closed, so the "single destination IP+domain overload" reason is outside
closeN() care or control. Neither the old nor the patched code tracks
"single destination IP+domain overload" conditions AFAICT.

If you think "any" is not good enough, how about the following more
explicit variant?

    /// closes any n connections, regardless of their destination
    void closeN(int n);

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

I agree that connections to no-longer valid IP addresses should be
closed when we can detect such IP addresses. Where do you think we
should add a TODO comment about that, peerDNSConfigure()?

Thank you,

Alex.
Received on Sun Apr 27 2014 - 17:20:19 MDT