Re: [PATCH] cleanup-comm

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

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

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

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

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

s/tehe/the/

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

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

Thank you,

Alex.
Received on Fri Jun 10 2011 - 17:41:52 MDT