Re: [PATCH] cache_peer standby=N

On 28/04/2014 5:19 a.m., Alex Rousskov wrote:
> 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",

The new statement quoted directly above "this".

The new documentation is implying the bug without being clear and as
such easily misinterpreted.

We should state the problem with idles clearly (yes it is difficult to
word), or we fix that problem (see below) and update the documentation
to match the working situation:
  s/standby/standby or idle/

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

FwdState::checkRetriable() relies completely on HttpRequest state and an
HttpRequest is already passed into peerHTTPOkay().

We can move FwdState::checkRetriable() to HttpRequest::retriable() and
have it used in peerHTTPOkay() to send its true/false result to
peerHasConnAvailable() about whether to count idles as available.

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

It is a limitation on its usage. It should not be used (or only with
great care under exceptional overload situations only) on the generic
idle pool for example, because it would terminate arbitrary outbound
connections outside the control of the caller.

NP: I am not objecting to the change, just requesting that it be clearer
documented about what the use limits/risks are.

>
> 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);
>

That reads a lot better.
I have a feeling it is still not enough, but dont have any better ideas.

>
>> 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()?

Yes that seems the right place to add a TODO.

Amos
Received on Mon Apr 28 2014 - 04:03:02 MDT