Re: [PATCH] cleanup-comm

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.

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

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.

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

I'm not so sure we should, mostly for the reasons displayed by the
example scenario above.

>
>> === modified file 'src/pconn.h'
>> + * Sorted oldest to newest for most efficient speeds on pop() and findUsable()
>
> It is not clear what connection age is in this context (or whatever the
> newest and oldest terms apply to). Consider rephrasing using standard
> "FIFO" or "LIFO" terminology.

Done. FIFO.

>
>
>> === renamed file 'src/ConnectionDetail.h' => 'src/comm/Connection.h'
>> + * Connection properties may be changed until tehe connection is opened.
>
> s/tehe/the/

Done.

>
>> === renamed file 'src/ConnectionDetail.h' => 'src/comm/Connection.h'
>> + * These objects must not be passed around directly,
>> + * but a Comm::ConnectionPointer must be passed instead.
>
> Not sure what you mean by "directly", but it seems like an exaggeration.
> Connection objects should not be copied (which we ensure by hiding copy
> constructor and assignment operator) but they can be passed around using
> references where no long-term storage is needed. There are many examples
> of by-reference passing in the code.

Yes we do that with the Comm::ConnectionPointer. Comm::Connection
performs socket operations on destruct.

s/must/should/

>
>> === 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?

* local/remote are not undefined. Just wildcard/any, which has meaning.

flags and ident I've now wrapped in if().

Thank you.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.12
   Beta testers wanted for 3.2.0.8 and 3.1.12.2