[squid-dev] [PATCH] helper queue polishing

[Amos Jeffries]

-----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 be 0.


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/

* 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.


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.

* 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.


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.


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).


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

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

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-----