[squid-dev] [PATCH] Response delay pools

[Eduard Bagdasaryan]

On 22.01.2017 22:52, Amos Jeffries wrote:

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

I assume that you are mixing up "pools" and "buckets". A pool may have
several "cascading" buckets. The existing server pools have several
buckets for classes > 1.  We can configure several server delay pools,
but only one first 'matched' pool is assigned to the transaction. Then
the "smallest" bucket of the matched pool determines the final speed
limiting (as you correctly noted).  Following this logic, old "client"
and new "response" pools should be independent (as if they would
represent different client pool classes), i.e., finally we
should select only one "first matched" client-side delay pool. Within
this selected pool, we select bucket with the smallest level (individual
or aggregate) similarly as we do for server pools of classes 2
or 3). However we do not do this selection if old client pool matched,
because it has the only one bucket.

 > ClientDelayConfig::parsePoolCount() please use emplace_back instead of
 > push_back.

AFAIK, emplace_back() would have a benefit over push_back if there was
an object itself (thus avoiding copying the object). In this case a
pointer is inserted so the benefit is negligible. I would prefer using
old push_back() here.


 > is there a specific reason this has to be a list of raw-pointers
 > instead of a list of objects like it was?

This change was a consequence of another fix: ClientDelayPool::access member
was not destroyed on shutdown causing "indirectly lost" Valgrind error 
messages.
ClientDelayPool class does not follow copy semantics(and probably should 
not)
due to acl_access pointer, requiring by stl vector. So I had to use 
pointers instead.
However my latest checks with test-builds.sh showed another linker problems,
because SquidConfig got unnecessary dependency on ClientDelayConfig
destructor. Finally I had to re-work ClientDelayConfig module,
introducing new ClientDelayPools class (a storage for ClientDelayPool
objects) and making ClientDelayPool ref-countable.

 >  which brings up a design contradiction: "why are there required
 > 'options'?
 > 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.

I agree that we should not require using all response_delay_pool
options.  For example, it would be useful to allow only "individual" or
"aggregate" options, e.g.:

#no aggregate limiting
response_delay_pool slowPool1  individual-restore=50000 \
                                   individual-maximum=100000

#no individual limiting, split aggregate bandwidth among clients
response_delay_pool slowPool2  aggregate-restore=100000  \
                                   aggregate-maximum=200000

No need for "none" world: just remove unneeded options (or specify -1
value). Using approach the existing server delay pools
adhere to ("none" for speed/maximum pair), we should not use "restore"
option without "maximum" for both "aggregate" and "individual" cases.
BTW, it is possible to specify "no limitation" at all for a pool
(i.e., no individual and aggregate limitation). The existing server
pools work similarly, allowing to specify "none" for all buckets
(without any warning message).

 > The limitation configured with initial_fill_level= was previously
 > configured through directive client_delay_initial_bucket_level.

"client_delay_initial_bucket_level" relates to only old client delay
pools.  I don't think we should reuse it in the new response pools: one
may need to configure both pools with different initial level parameter.


Thanks for the detailed review. I tried to address all other remarks,
renamed parameters according to the suggested terminology,
merged with latest v5 r15027 and re-attached the patch.


Eduard.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: SQUID-50-response-delay-pools-t3.patch
Type: text/x-patch
Size: 101019 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20170131/7a9b66c7/attachment-0001.bin>