Re: [PATCH] Support client connection annotation by helpers via clt_conn_id=ID

On 20/06/2014 6:07 a.m., Tsantilas Christos wrote:
> On 06/16/2014 06:36 PM, Alex Rousskov wrote:
>> On 06/15/2014 12:07 AM, Amos Jeffries wrote:
>>> On 15/06/2014 4:58 a.m., Alex Rousskov wrote:
>>>> On 06/11/2014 08:52 AM, Tsantilas Christos wrote:
>>>>
>>>>> I must also note that this patch adds an inconsistency. All annotation
>>>>> key=values pairs received from helpers, accumulated to the
>>>>> existing key
>>>>> notes values. The clt_conn_id=Id pair is always unique and replaces
>>>>> the
>>>>> existing clt_conn_id=Id annotation pair.
>>>>> We may want to make all annotations unique, or maybe implement a
>>>>> configuration mechanism to define which annotations are overwriting
>>>>> their previous values and which appending the new values.
>>>>
>>>> I suggest making all annotations unique (i.e., new values overwrite old
>>>> ones) because helpers that want to accumulate annotation values can do
>>>> that by returning old values along with new ones:
>>>>
>>>> received by helper: name=v1
>>>> returned by helper: name=v1,v2
>>>>
>>>> Please note that the opposite does not work: If annotation values are
>>>> always accumulated, a helper cannot overwrite/remove the old value.
>>
>>
>>> Doing that would mean passing all existing annotations to every helper
>>> lookup.
>>
>> Why would that mean the above?

All helpers may return message= or log=. Replacing existing notes means
we have to pass at least those to every helper explicitly.

>>
>> AFAICT, the helper should get only the annotations it needs. That need
>> is helper-specific and, hence, is configurable via the various _extras
>> and equivalent directives. That is already supported and does not need
>> to change.

Ideally yes. However a helper requiring previous helpers annotations
because it *might* replace them is a new detail created by this proposed
replacment model. In the existing model of appending notes the secondary
helper only requires what it needs for its core action logics.

With the message= example, any 1-byte variance on first helpers message=
output will result in extra churn on the second helpers response cache
and at least an extra lookup loading on the helper.

>>
>> Here is the overall sketch for supporting "unique annotations":
>>
>> 1. Send the helper the annotations it is configured to get
>> (no changes here).
>>
>> 2. For each unique annotation key received from the helper,
>> remove any old annotation(s) with the same key.
>>
>> 3. Store annotations received from the helper
>> (no changes here).
>>
>> To support explicit annotation deletion, we can adjust #3 to skip
>> key-value pairs with the value equal to '-'.
>
> If there is not any objection I will implement this scenario.
>
> Looks that this approach is the best and cover more cases than the
> accumulated Notes values.
> If someones need to accumulate Note values he can configure squid to
> send old note value to helper and helper include it in its response.
> This is simple.

The only case it adds coverage for is giving helpers ability to erase
previous helpers annotations. While reducing the helper response cache
HIT ratio.

That said, I am okay with the design change. AFAIK there is nobody
making much complicated use of helper notes (yet).

Amos
Received on Fri Jun 20 2014 - 05:46:56 MDT