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

On 07/01/2011 02:44 PM, Tsantilas Christos wrote:
> On 07/01/2011 12:07 PM, Tsantilas Christos wrote:
>> 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
>
> The 4th version of the patch, which implements the above .
> It still needs some testing, I am posting it here in the case someone
> wants to test it.

> - theIdleConns("ICAP Service",NULL),

Please initialize to NULL in case we throw before we reach the
allocation lines below.

> + theIdleConns = new IdleConnList("ICAP Service",NULL);
> + assert(theIdleConns);

Please remove the assert(). The new operator cannot return NULL.

> + if ((reused = Comm::IsConnOpen(connection))) {
> debugs(93,3, HERE << "reused pconn " << connection);
> - ++theBusyConns;
> }
> + ++theBusyConns;
>
> return connection;
> }

Unless I am missing something, this can be simplified and optimized as:

    ++theBusyConns;
    debugs(93,3, HERE << "got connection: " << connection);
    return connection;

When the connection is printed, we will see whether it is open, right?

The above changes are cosmetic and should not block the commit if the
patch passes your tests, of course.

Thank you,

Alex.
Received on Mon Jul 11 2011 - 23:43:26 MDT