Re: [PATCH] cleanup-comm

On 06/10/2011 02:00 PM, Amos Jeffries wrote:
> On 11/06/11 05:40, Alex Rousskov wrote:
>> On 06/04/2011 08:05 AM, Amos Jeffries wrote:
>>> * re-constructed the Comm::Connection handling in ICAP after
>>> max-connections patch. There is a pconn key problem now.
>>>
>>> The new model of pconn pool requires remote IP:port for the key. ICAP
>>> does not have any IP visibly available in closeN() where 3.HEAD
>>> pre-emptively purges idle pconn to a smaller max. So this patch breaks
>>> that optimization. Existing connections will still be closed without
>>> re-pooling as requests finish with them.
>>
>>> I don't see this as a major issue to block the patch merge. But would
>>> like to fix it ASAP if anyone can point out a simple change to do so.
>>
>>
>>> === modified file 'src/adaptation/icap/ServiceRep.cc'
>>> + // XXX: this whole feature bases on the false assumption a
>>> service only has one IP
>>> setMaxConnections();
>>
>> I disagree with this XXX. The Max-Connections feature assumes that an
>> ICAP service with multiple IPs can pool all its connections together
>> because they are all going to the same service. It does not assume that
>> a service cannot have more than one IP address (although that seems to
>> be rather rare for other reasons).
>
> OPTIONS appears to be defined *globaly* for the service by one instance.
> With detais like features this is reasonable.
> With details like network tuning this assumes a precise monoculture
> cluster or a singleton instance.

Agreed. This is just how ICAP works. There is no XXX (i.e., problem or
bug in Squid) here.

It would be rather difficult/costly to correctly implement a single ICAP
service that uses multiple IP addresses, supports Max-Connections, and
runs on multiple boxes. There are other ICAP features that are difficult
to support in distributed services. In my experience, folks tend to
load-balance independent ICAP services rather than creating one
distributed ICAP service.

>>> === modified file 'src/adaptation/icap/ServiceRep.cc'
>>> const int excess = excessConnections();
>>> // if we owe connections and have idle pconns, close the latter
>>> + // XXX: but ... idle pconn to *where*?
>>
>> Similar to the above, the answer is "to the service". All connections to
>> the service are treated the same. Why is that wrong?
>
> So for consideration. An example;
> "The Service" being 10 pieces of hardware each with limited max
> capacity of 1000 conn.

You are describing a setup that ICAP Max-Connections does not support
and ICAP, in general, was not designed for. So the questions below
simply do not have answers other than "change your design" or "do not do
it".

> Does OPTIONS max-connection contain 1000 or 10,000?
>
> ... if it was 10,000 is it okay for all 10,000 to go to just one of
> those hardware?
>
> ... if it was 1000, is it okay for 9 hardware to be bypassed claiming
> "overloaded" while they sleep with 0 connections?
>
> ... what happens when we plug in 5 more boxes with twice the individual
> capacity?
>
> ... what happens if someone typos "100" on just one instances limit config?

> With the new design of idle pool indexed by destination IP we can easily
> avoid all these and maximize the total service capacity.

Except we break ICAP that does not have a concept of a service IP
address (on this level). I do not know what problem by-IP pool design
solves for other protocols, but it seems to be not needed for ICAP.

>>> === modified file 'src/adaptation/icap/ServiceRep.cc'
>>> if (excess&& theIdleConns.count()> 0) {
>>> const int n = min(excess, theIdleConns.count());
>>> debugs(93,5, HERE<< "closing "<< n<< " pconns to relief
>>> debt");
>>> - Ip::Address anyAddr;
>>> - theIdleConns.closeN(n, cfg().host.termedBuf(), cfg().port,
>>> NULL, anyAddr);
>>> + theIdleConns.closeN(n, Comm::ConnectionPointer(),
>>> cfg().host.termedBuf());
>>> }
>>
>> Can you adjust PconnPool::pop() so that if destLink is NULL, any
>> destination IP would match? If yes, then the above would still work as
>> intended, I think.
>
> Possibly. Provided we drop the earlier discussed ideas of merging the
> ICAP and HTTP connections pools.

Merging in what way? We already use the same pooling classes for both
ICAP and HTTP, right?

Also, what would break if NULL destLink matches any IP for both ICAP and
HTTP?

(Perhaps we should use "any IP" destLink instead of NULL destLink to
make this less of a special case and more close to regular IP semantics?)

>>> === renamed file 'src/ConnectionDetail.h' => 'src/comm/Connection.h'
>>> +inline std::ostream&
>>> +operator<< (std::ostream&os, const Comm::Connection&conn)
>>> +{
>>> + os<< "FD "<< conn.fd<< " local="<< conn.local<<
>>> + " remote="<< conn.remote<< " flags="<< conn.flags;
>>> +#if USE_IDENT
>>> + os<< " IDENT::"<< conn.rfc931;
>>> +#endif
>>
>> Perhaps we should avoid printing undefined/unset members, to reduce the
>> amount of noise in the debugging logs?
>
> Sure.
>
> * FD -1 /was kind of nice to see closed connections. Do we want to drop
> that?

I think we should drop printing negative FD. The connection is closed
when it shows no FD so we are not losing any information here.

This is not a big deal, of course, and, eventually, we should add
Connection::id which will always be there.

Thank you,

Alex.
Received on Sat Jun 11 2011 - 12:58:06 MDT