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.
