Re: [PATCH] cleanup-comm

On 06/11/2011 07:51 AM, Amos Jeffries wrote:
> On 12/06/11 00:57, Alex Rousskov wrote:
>>
>>>>> === 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?
>
> We have a few emails about using one PconnPool set and using it to
> push/pop all idle TCP links. With ICAP using a different type of keys
> as HTTP this becomes a dead idea.

I am not so sure: A single pool could make sense if we want to remove
the oldest idle connection (no matter to what destination) before
opening a new one (to some specific destination). Whether that is a good
idea is a separate question, but this can be implemented even with keys
of different "type".

On the other hand, it may be better to have many dedicated pools like
today, but also add a single/global index for the "remove the oldest
idle connection" operation. That order specified by that index will not
be determined by the key.

> I have the idea we should make each Service have its own IdlePool.
> Dropping the use of a shared ICAP PconnPool. That way none of the
> operations have to bother with keys and hashing calculations.
>
> What think you?

I think Service already has its own idle pconn pool: theIdleConns.
Unfortunately, the PconnPool class does not allow us to disable or
customize connection indexing yet, so we have to inherit all those
calculations that we do not need.

Cheers,

Alex.
Received on Tue Jun 14 2011 - 18:24:46 MDT