[squid-dev] [PATCH] helper queue polishing

[Tsantilas Christos]

On 11/04/2014 03:52 AM, Amos Jeffries wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 4/11/2014 8:05 a.m., Tsantilas Christos wrote:
>> This patch try to polish helpers queue to: 1. Make the queue limit
>> configurable, with the default set to 2*n_max. 2. Move common queue
>> limit checks inside helper.cc. 3. Make all code to use those new
>> helper features.
>>
>> A first discussion for such changes done on squid-dev on a
>> mail-thread under the subject "[PATCH] Prevent external_acl.cc
>> "inBackground" assertion on queue overloads" :
>> http://www.squid-cache.org/mail-archive/squid-dev/201301/0083.html
>>
>> For more informations please read documentation in patch preamble.
>>
>> This is a Measurement Factory project
>>
>
> in src/redirect.cc:
> * the change to redirector bypass is undocumented. The bypass was
> previously a bypass of queueing as well as helper. If we now wait
> until the queue is full then we have not bypassed the queueing.
>   - I think for backward compatibility redirector_bypass should
> implicitly set default queue-size to b

OK for this.
It requires to add a Helper::ChildConfig::defaultQueueSize member which 
I do not like it, but it is not bad....

>
>
> in cf.data.pre:
> * "overloading persist squid may abort its operation" for
> url_rewrite_children and store_id_children
>    - s/persist/persists/
>
> * "then storeID helper bypassed"
>    s/bypassed/is bypassed/

ok for these changes

>
> * need to document how the redirector/storeid helper bypass has changed.
>   - when bypass AND queue-size>0 are configured a queue exists, older
> config the queue was always bypassed.

It should be ok now...
I document these changes in patch preameble, and I will add them when I 
commit the patch to trunk. Do we need such documentation on 
release-3.6.sgml file? I put here only basic documentation.

>
>
> in src/external_acl.cc:
> * there is no need for the temporary local int queue_size
>    - use atoi() to set children.queue_size member and adjust later with
> if condition like the others.

ok

>
> * in the if-condition calling trySubmit() the debugs outputs a
> hard-coded "queue is too long" error. This is an assumption which will
> not always be true - there are many reasons a submit may fail to
> submit (helper dying early, shutting down, etc).
>   - the right place to output that error is inside trySubmit() where I
> see it already does so.
>   - if a debugs is needed on trySubmit() failure it should only say
> that submit failed. Cannot say why exactly.

I made the debugs simpler.....

>
>
> in src/src/helper/ChildConfig.cc:
> * the second constructor is passed n_max value to use. Should be
> setting default queue-size to 2*m at that point instead of 0 for
> helpers which are not configured explicitly with children parameters.

fixed

>
>
> in src/helper.cc: (optional but desirable to reduce code)
> * why are you retaining the legacy helperSubmit() now that helpers all
> have a members?
>   - the helpers still using old queue limits should just have
> queue_size hard-coded to those limits and use the new API methods.
>
> * helperStatefulSubmit() is/was only used by Negotiate and NTLM auth
> helpers which you have documented as supporting the new queue-size. So
> there should be no need for helperStatefulSubmit() to exist now.
>   - you can probably remove both these wrappers by making submit()
> virtual, and both submit() and trySubmit() receive a 'lastserver'
> parameter (default of NULL for it).

I prefer to not make these changes now...
The helper and statefullhelper classes need to be "more" c++ classes and 
their code merged if possible. But this is an other project....

>
>
> Also:
>   * missing doc/release-notesic /release-3.6.sgml entries for the updated
> config directives.

I put only basic documentation. I did not put the helper behaviour 
changes. Someone can find them when reads the squid.conf.
Is it OK?

>
> * please take care updatign to current trunk, CBDATA details have been
> altered around some of these chunks.

ok.

>
> Amos
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.22 (MingW32)
>
> iQEcBAEBAgAGBQJUWDFhAAoJELJo5wb/XPRjjDIH/2xPrVJ6/Fegayh3jcRtR/VK
> Ru/NzM9Rh5/UM1zTmHzknR+nrGoK+XOOA5c3E6MVj4+KI1o5PycRHNQlTGcqfnYO
> AcOi3qRwbLmVTMty+mz1mmV/AFs73IKP4yPYkThb861ySo00Pb9WsPkJ0NVh3jon
> RdxROoPkFPGimmwEiRUqpu8y8JuuxfsW/tJEwR8nLCdR0vbVbszTMLGrdiYPNOQ1
> NfN2i5+z5S0sCz9qYPRYrvduwdboawGBCZ7GxQl0TsoJG2YDqZ20UUOwNImtlOFV
> XB0T+zVtet10EQXHkMsuE3danN2aHQMqrxSgVF1htkRknp5WuneXyLju3wWPuIw=
> =Brjc
> -----END PGP SIGNATURE-----
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: helper-queue-polishing-t4.patch
Type: text/x-patch
Size: 56922 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20141105/02269242/attachment-0001.bin>