[squid-dev] [PATCH] Response delay pools

[Amos Jeffries]

On 23/01/2017 12:10 a.m., Eduard Bagdasaryan wrote:
> Hello,
> 
> This patch introduces a new "response_delay_pools" feature. I have
> posted it(with detailed description) recently to the thread for
> preliminary review with "preview" flag.  This patch conforms to latest
> v5 (r15011) and also has some problems fixed:
> 
> * MessageBucket::theAggregate pointer became invalid during
>   reconfiguration.
> 
> * Response delay pools got stuck after reconfiguration, consuming
>   no more data.
> 
> * A performance improvement: MessageDelayPools::Update() was called
>   every second(via eventAdd) consuming CPU cycles even when no
>   connections were served.
> 


I am a little bit worried about this behaviour:
 "It is possible that a response becomes a
subject of both these pools. In such situations only matched response
delay pool will be used for Squid-to-client speed limiting."

... as I understood it the existing delay pools design is that multiple
pools can apply to traffic. In which case on each write() attempt the
bucket with smallest available amount determins the write size and all
buckets have the actually consumed amount removed.


* Please include in the commit description your explanation of why this
is needed. The text which you posted to the PREVIEW thread for me.


in src/BandwidthBucket.cc:

* please remove the extern definition for current_dtime.
 - include SquidTime.h if actually needed.
 - There are other time API things being used so you may not need that
include anyway.

* in answer to the "XXX: Decide whether to add 'hash' field like
ClientInfo::hash"
 - that should be a TODO at most. its not a critical missing feature if
you can comment out the debugs by default.
 - also the answer is 'no'. we are trying to remove hash.h entirely. so
there probably wont be an available value when anyone gets around to
fixing the XXX/TODO.
 - just remove that XXX and maybe the commented-out debugs. If its not
worth printing now, dont bother coding it.
 - if you do want something to track what the bucket is. then use a
'this' pointer value as the ID, or an InstanceId class member.

* and of course remove HERE macro from all the shuffled debugs lines.
There are a few, probably from where you have cut-n-paste'd code around,
but that counts as changing.


in src/ClientDelayConfig.cc:

* can you add the missing "WARNING:" label in the DBG_IMPORTANT message
produced by ClientDelayConfig::finalize() please.

* ClientDelayConfig::parsePoolCount() please use emplace_back instead of
push_back.
 - is there a specific reason this has to be a list of raw-pointers
instead of a list of objects like it was?


in src/Makefile.am:

* I'm trying to keep the SOURCE* lists alphabetical as much as possible.
 - please place the new files in the 'B' and 'M' sections of
DELAY_POOL_ALL_SOURCE appropriately.


in src/MessageBucket.cc

* Please use MEMPROXY_CLASS instead of custom new/delete operators for
MessageBucket class. My patch cleaning that up for the other classes
will be going in shortly.


in src/MessageBucket.h:

 * the "namespace Comm" definitions are things that comm/forward.h
should be providing.
 - as a rule of thumb: if any forward.h file is not providing API
pre-definitions properly please fix that instead of pre-defining elsewhere.


in src/MessageDelayPools.cc:

* please place the system includes <foo> in a separate block of includes
listed _after_ the local includes "foo"

* MessageDelayPools::add() method is missing a "WARNING:" label on the
debugs(..., DBG_CRITICAL, "Ignoring duplicate" ...


* MessageDelayConfig::parseResponseDelayPool() method debug errors:
 - messages which are followed by self_destruct() need to say FATAL not
ERROR.
 - when the directive name is available we start config errors by naming
it then the describing what is wrong with the line.
eg:
  "FATAL: response_delay_pool missing required 'name' parameter");
  "FATAL: response_delay_pool unknown option '" << token << "'");
  "FATAL: response_delay_pool missing required option '" << failedOption
<< "'");


* which brings up a design contradiction: "why are there required
'options'?" options are optional by definition.
 - a config line should have roughly this generic structure:
   1) directive (first label, which determines the syntax)
   2) parameters (mandatory values, fixed positions for each one)
   3) options (values that may be absent, using key[=value] syntax).
   4) acl-list (if relevant)

 - so the answer is that they should not be required, the value -1 in
pools just means "no limit" or "infinity".
 - the for-loop checking for missing options should be removed and use
of the "none" magic word as value for these options should be supported.
 - we use the value of -1 for 'none' internally in the config settings.


* Please use '-' instead of '_' in option and parameter names. It avoids
confusion with directive names.

* Please use static constants for the parameter name SBuf and the
std::map instead of dynamically allocating on each call.

* Please also use the existing pools terminology for naming these
parameters.
 - I know its not perfect, but the existing terms are already understood
and documented.
 - s/bucket_speed_limit=/individual-restore=/
 - s/max_bucket_size=/individual-capacity=/
 - s/aggregate_speed_limit=/aggregate-restore=/
 - s/max_aggregate_size=/aggregate-capacity=/

* The limitation configured with initial_fill_level= was previously
configured through directive client_delay_initial_bucket_level.
 - you dont seem to be deprecating that directive. So please either make
these new pools obey it and remove the initial_fill_level= option,
OR,
 - s/initial_fill_level/initial-bucket-level=/ and use the generic
directive as its default value (may be -1 for 'none').


in src/cache_cf.cc:

* Please move the free_*/parse_*/dump_* macros and wrappers into the
relevant ClientDelayConfig.h or MessageDelayPools.h after the config
class itself is defined.
 - they can be inline functions and reduce the amount of cruft in
cache_cf.cc.


in src/cf.data.pre:

* Please remove the new "RESPONSE DELAY POOL PARAMETERS" section header
 - being under "CLIENT DELAY PAREMETERS" section is sufficient and
correct for what these directives do (client I/O delays).

* s/squid.conffor/squid.conf for/

* for the description of response_delay_pool please add a reference to
delay_parameters for the terminology descriptions.
 - and re-write the option descriptions to match the terminology.
 - and also to match the changes requested above regarding 'none' and
options being optional.

* for the description of response_delay_pool_access please consider:
"
	Determines whether a specific named response delay pool is used
	for the transaction.

	This feature restricts Squid-to-client bandwidth only.

	The syntax for this directive is:

	  response_delay_pool_access pool_name allow|deny acl_name

	All response_delay_pool_access options are checked in the order
	they appear in this configuration file. The first rule with a
	matching ACL wins.

	If (and only if) an "allow" rule won, Squid restricts the
	response using the corresponding named delay pool.
"

Amos