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

Any comment on this patch?
Can I commit it to trunk?

On 05/09/2011 12:41 PM, Tsantilas Christos wrote:
> 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
>
Received on Fri May 13 2011 - 08:27:05 MDT