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

On 06/23/2014 09:50 AM, Amos Jeffries wrote:
> On 23/06/2014 5:43 a.m., Tsantilas Christos wrote:
>> Hi all,
>>
>> The attached patch replaces existin annotation values with the new one
>> received from helpers.
>>
>> Just one question. We are documenting key-value pairs in cf.data.pre
>> only for url-rewriter helpers, but annotations works for all squid helpers.
>> Should we move the related url-rewriter section to a global section? If
>> yes where?
>>
>> For example something like the following in a global section should be
>> enough:
>>
>> The interface for all helpers has been extended to support arbitrary
>> lists of key=value pairs, with the syntax key=value . Some keys have
>> special meaning to Squid, as documented here.
>>
>> Currently Squid understands the following optional key=value pairs
>> received from URL rewriters:
>> clt_conn_id=ID
>> Associates the received ID with the client TCP connection.
>> The clt_conn_id=ID pair is treated as a regular annotation but
>> it persists across all transactions on the client connection
>> rather than disappearing after the current request. A helper
>> may update the client connection ID value during subsequent
>> requests by returning a new ID value. To send the connection
>> ID to the URL rewriter, use url_rewrite_extras:
>> url_rewrite_extras clt_conn_id=%{clt_conn_id}note ...
>>
>
> Design issue:
> * can we call this "client-tag=" or "connection-tag=" or
> "connection-label=". We have ID used internally via InstanceId<>
> template members, which are very different to this "ID" being user-assigned.

The clt_conn_id is not a client tag, but it can be a connection-tag or
connection-label.
Lets wait Alex to answer on this.

>
>
> in src/Notes.cc:
> * replaceOrAdd iterates over the notes list twice in the event of
> removals (due to remove() iterating over it from scratch). Please fix
> this to iterate over the list only once and do an in-place replace if
> the key is found already there, add if end() is reached.
>
> * remove() appears to be unnecessary after the above correction.

I can fix a litle replaceOrAdd, but the remove() method still is needed.

The replaceOrAdd() method method will be:
void
NotePairs::replaceOrAdd(const NotePairs *src)
{
  for (std::vector<NotePairs::Entry *>::const_iterator i =
src->entries.begin(); i != src->entries.end(); ++i) {
         remove((*i)->name.termedBuf());
         entries.push_back(new NotePairs::Entry((*i)->name.termedBuf(),
(*i)->value.termedBuf()));
     }
}

>
>
> in Notes.*:
> * findFirst() no longer makes sense. There being no "second" now.
> - Please rename to find().

This is not true. We can still support multiple values per key name.
I am suggesting to not touch this one.

>
>
> in src/cf.data.pre:
> * "kv-pairs" is a format syntax definition used throughout the
> documentation. Please retain it. If the term needs re-defining that is
> an issue outside this patch scope.
>
> * Please reword "Squid understands the following optional key=value
> pairs received from URL rewriters:". Since there are optional kv-pairs
> already documented in the rewrite vs redirect vs no-change syntax we
> need to specify that these are optional extensions.
> Perhapse this:
> "In addition to the above kv-pairs Squid also understands the following
> optional kv-pairs received from URL rewriters:"
>
> * Please update the kv-pair description like so (where FOO is the term
> tag or label selected to replace ID by the above mentioned design issue)
> "
> Associates a FOO with the client TCP connection.
> The FOO is treated as a regular annotation but persists
> across future requests on the client connection
> rather than just the current request.
> A helper may update the FOO during subsequent
> requests by returning a new kv-pair.
> "
>
> * Reference to url_rewrite_extras should be directy after the helper
> input syntax line. Like so:
> "
> [channel-ID <SP>] URL [<SP> extras]<NL>
>
> see url_rewrite_extras on how to send "extras" with
> optional values to the helper.
>
> "

Ok for these changes

>
>
> in src/client_side_request.cc:
> * ClientRequestContext::clientRedirectDone was previously calling
> SyncNotes to ensure the AccessLogEntry and HttpRequest shared a notes
> list before updating the request. Order is important here because Sync
> will kill Squid with an assert if the objects have different notes list
> objects.
> Until that is fixed please retain the order of:
> - sync first, then update the request.
>
> * same in ClientRequestContext::clientStoreIdDone

It is the same, it is not make difference.
UpdateRequestNotes will just copy helper notes to the request notes. The
SyncNotes will check if AccessLogEntry and HttpRequest object points to
the same NotePairs object.

>
> * in ClientHttpRequest::doCallouts() please use SBuf::isEmpty() instead
> of length() to check for existence.

OK.

Lets wait others comments before I post a new patch.

>
>
> Amos
>
>
>
Received on Mon Jun 23 2014 - 13:45:05 MDT