Re: [PATCH] HelperReply upgrade stage 3

On 11/30/2012 04:28 AM, Amos Jeffries wrote:
> Updated patch attached for review.

> + if (!helperNotes) {
> + delete helperNotes;
> + helperNotes = NULL;
> + }

Wrong condition.

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

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

Thank you,

Alex.
Received on Fri Nov 30 2012 - 16:56:49 MST