Re: [PATCH] Service Overload Handling (ICAP Max-Connections)

A fixed patch.

On 05/08/2011 06:46 AM, Amos Jeffries wrote:
> On 27/04/11 23:40, Tsantilas Christos wrote:
>> This patch implements the "Service Overload Handling" feature as
>> described in the squid wiki page:
>> http://wiki.squid-cache.org/Features/ServiceOverload
>>
>> This feature also exist as bug #2055 in the squid bugzilla:
>> http://bugs.squid-cache.org/show_bug.cgi?id=2055
>>
>> More informations included in the patch preamble.
>>
>> Regards,
>> Christos
>
> Some relatively minor tweaks in the parse handling:
>
> Adaptation::ServiceConfig::grokLong(
> Adaptation::ServiceConfig::grokOnOverload(
> Adaptation::ServiceConfig::grokBool(
>
> - Please use DBG_CRITICAL instead of ", 0,". Or if it is not critical
> bump to an appropriate alternative (parsing errors are usually
> DBG_IMPORTANT).
>
> - use "ERROR:", "FATAL:","WARNING:" as appropriate to the seriousness of
> the config problem.
>
> * For parsing problem debugs please just use "cfg_filename" instead of
> "HERE << cfg_filename".

I fixed a little the ServiceConfig::grokLong and
ServiceConfig::grokOnOverload methods.
I will fix the ::grokBool method with a separate patch after
max-connections patch commited.

>
>
> One *Major* problem:
>
> Adaptation::Icap::ServiceRep::getConnection() is missing IP split-stack
> support removed from Adaptation::Icap::Xaction::openConnection().

Agrr
I missed this one....

>
> On systems where IPv6 sockets can be opened but the ICAP server is
> IPv4-only (MacOSX, OpenBSD, Windows, manually disabled kernels) this
> will fail to connect() the socket.
>
> You will need to preserve the ipv6=on|off option behaviour. Like so:
>
> Ip::Address anyAddr; // default: IP6_ANY_ADDR
> ...
> // need a new connection
> if (!reuse) {
> if (!Ip::EnableIpv6)
> anyAddr.SetIPv4();
> else if (Ip::EnableIpv6&IPV6_SPECIAL_SPLITSTACK && !cfg().ipv6)
> /* split-stack for now requires default IPv4-only socket */
> anyAddr.SetIPv4();

We are assuming that Address::SetIPv4 will always return true for
ANY_ADDR...

Maybe we can use
if (!Ip::EnableIpv6 || Ip::EnableIpv6&IPV6_SPECIAL_SPLITSTACK &&
!cfg().ipv6))

But I let it as you suggested just for the "split-stact for now..." comment

>
> connection = comm_open(SOCK_STREAM, 0, anyAddr, COMM_NONBLOCKING,
> cfg().uri.termedBuf());
> }
> ...
>
>
> NP: the failover logics in comm_openex() are for handling sockets with
> tcp_outgoing_addr pre-known. *_ANY_ADDR will never fail based on family
> and so will never failover.
>
> Amos