Re: [PATCH] HelperReply upgrade stage 3

On 1/12/2012 5:56 a.m., Alex Rousskov wrote:
> On 11/30/2012 04:28 AM, Amos Jeffries wrote:
>> Updated patch attached for review.
>> + if (!helperNotes) {
>> + delete helperNotes;
>> + helperNotes = NULL;
>> + }
> Wrong condition.

:-( fixed.

>> + Notes *helperNotes; // collection of meta notes associated with this request.
> Please use doxygen comments.
>
> Consider s/meta notes/annotations/.
>
> If we are convinced that the set of notes for the original and cloned
> requests should be the same, consider making this a refcounted pointer
> and avoid allocating new helperNotes in clone(). If the "all requests
> have same notes" assumption holds, this will avoid making ICAP (and
> possibly other paths) performance [slightly] worse when helper
> annotations are present.

Its not that the request objects should all be the same so much as the
clone should get all the parents one. The clone can still be added to by
itself and that should not affect the original request it was cloned
from (think external ACL notes from http_reply_access).

>
>>> The description seem to imply that metaNotes contains all annotations
>>> associated with this request, but I do not think this is true because
>>> some annotations are stored elsewhere. This is messy so let's document
>>> exactly which annotations are using this particular storage (I think it
>>> is currently used just for helper annotations, right?).
>> It *will* contain all the notes from all the helpers - when I get
>> around to altering the other helpers. But this sponsor had specific
>> target requirements and a short timeframe I did not get to make the
>> same update on the other interfaces code (yet).
> It was not clear whether the data member contained all notes or just
> notes from the helpers (just some helpers now and eventually all
> helpers, that distinction does not matter IMO). It is clear now.
>
>
> IIRC, we currently have these annotations (of all kinds):
>
> * tags from external ACLs
> * notes from URL rewrite helpers
> * notes from the note directive
> * meta headers from adaptation_meta directive
> * meta headers from adaptation service responses
>
>
> And thank you for addressing my previous concerns. I am somewhat
> uncomfortable with notes sharing among clones requests, but you have
> added appropriate comments and since you do not see any specific problem
> with such sharing, I think we should try it.
>
> No further review is necessary as far as I am concerned.

Cheers. Merged to trunk after the above fix.

Amos
Received on Fri Nov 30 2012 - 23:10:24 MST