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
