Re: [PATCH] Add DNS lookup step to ICAP connect

On 07/01/2011 04:09 AM, Amos Jeffries wrote:
> On 01/07/11 11:53, Alex Rousskov wrote:
>> On 06/30/2011 11:11 AM, Tsantilas Christos wrote:
>>> And this is the third version.
>>>
>>> This patch does not include only the DNS lookup for ICAP but other
>>> changes too like ICAP connections pool changes, which required to allow
>>> ICAP client work.
>>
>> Why do we need to dynamically allocate theIdleConns? Just curious...
>>
>
> The connections pointed to are RefCounted.
>
>>
>>> This patch also is not complete. The Max-Connections ICAP feature is
>>> currently broken and needs some redesign, to work well.
>
> ?! How is it broken? Alex and I just had a major discussion making sure
> the open/close conditions were identical now to prior the comm changes.

The Max-Connections behaviour in squid currently controlled by the
"ServiceRep::theBusyConns" which counts the Active connections on ICAP
server.

The latest patch I sent:
   - Increase the "theBusyConns" counter when the ServiceRep retrieves a
connection from idle connections pool, inside ServiceRep::getConnection
method.
   - Increase the "theBusyConns" counter when a new connections to the
ICAP server established using the ServiceRep::noteConnectionUse (this
one was added to the v3 patch)
   - decrease the "theBusyConns" counter inside
ServiceRep::putConnection which called after the Xaction completes to
pass the connection object back to ServiceRep. The ServiceRep will
decide if the connection will be closed or stored on theIdleConns
connection pool.

The above looks correct but it has a major problem. When a new "open
connection" job scheduled we do not know how many such jobs scheduled
until the connections established. We may scheduled more than the
Max-Connections limit. It is very easy to overload an ICAP service.

The above can be modified as follows:
- Always increase the "theBusyConns" counter inside
"ServiceRep::getConnection" method (even if we did not return a connection)
- (1)Define a new method "ServiceRep::noteConnectionFailed" which will
decrease the "theBusyConns" counter and will be called from Xaction when
the connection failed OR (2) move the "open-connection" logic in
ServiceRep, where the theBusyConns will be decreased on connection
failures. (The second maybe is not easy...)
- Decrease the "theBusyConns" counter inside ServiceRep::putConnection

Also we need to check under what circumstances the
Xaction::closeConnection() will not call the ServiceRep::putConnection,
or the Xaction::closeConnection will not called if a connections is
already opened (eg on Xaction failure), if there are such cases.

>
>>
>> Sigh. We just got it in!
>>
>>
>>> I think we should move the DNS-lookup and open connection to the ICAP
>>> server functionality from Adaptation::Icap::Xaction class to
>>> Adaptation::Icap::ServiceRep class.
>>
>> I am fine with that if you think it is going to make things work again.
>> Just be careful when passing the open connection object from Service to
>> Xaction. If Xaction is gone, make sure the connection is returned to the
>> Service or, at least, closed. IIRC, you already have a custom AsyncCall
>> or Dialer that does similar stuff so you can reuse/adjust that code.
>>
>
> getConnection() provided by the ServiceRep is currently producing
> connections synchronously for Xaction. That will need changing, then it
> will be trivial to both get pre-opened connections to Xaction and to
> store the list of potential/closed new ones in ServiceRep for use by
> other Xaction's. Thus load-balancing across all DNS results.
>
> Amos
Received on Fri Jul 01 2011 - 09:07:35 MDT