Re: [PATCH] cleanup-comm

 On Tue, 14 Jun 2011 12:23:46 -0600, Alex Rousskov wrote:
> 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.

 sorry to be accurate I meant IdleConnList, not 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.

 Right a full PconnPool per service. One PconnPool being a hash of
 multiple sets of FD.

 Since you seemed previously to believe a PconnPool was singular. I'll
 run with that and present the proposed patch in new thread "ICAP pconn
 optimization"

 Amos
Received on Wed Jun 15 2011 - 01:08:08 MDT