Re: [PREVIEW] StoreID stage 2\3 fully functional StoreID interface.

Hey,

I had a delay and I hope the next preview is an improvement to the old one.
I know I'm out of sync from trunk and once the preview is OK I will
present a patch in sync with trunk.

On 12/06/2012 03:20 AM, Amos Jeffries wrote:
> On 06.12.2012 04:24, Eliezer Croitoru wrote:
>> This patch implements the StoreID fully functional but yet to take in
>> full account PURGE\ICP\HTCP requests.
>>
>> The patch dose not apply the "mem-obj->url" change to
>> "mem-obj->store_id" since it's not needed for now but will be done
>> later.
>>
>> I wasnt sure about couple things so I left notes inside the code
>> about them.
>
> I found *one*. Please mark such questions with XXX or FIXME to make it
> more obvious for us that is a questionable action to be verified/fixed
> rather than documentated reason for an action being taken.

* Will mark them
>
>>
>> I am here for questions about anything.
>> There are things that I might not took in account and this is what
>> the patch here for.
>
>
> Audit results:
>
> in src/HttpRequest.cc:
> - missing empty line before the documentation on HttpRequest::storeId().
> - also please wrap the return type of HttpRequest::storeId() to its
> own line like the rest of Squid code.
V
> - generating the canonical URL is not necessary unless it is the
> result to be returned. The urlCanonical is also optimized for
> performance. So please simply call it twice (once for the debugs
> display, and once for the return statement.). That removes the need to
> allocate a buffer for String can variable.
> - also the 'this' member of an HttpRequest object should not need
> casting to 'HttpRequest*'. Please try removing the static_cast. If you
> have compiler errors after that please show them to use to figure out
> what is actually needed (I suspect a const_cast is more appropriate if
> anything).
> - please remove HERE from the new debugs() messages. This goes for
> any new code on squid-3.3+
> - please remove the "storeId:" label from the debugs messages, it is
> added by default with the HERE pieces.
> - please remove the "is being used" debugs line entirely, it is
> redundant with the other two which cover the function usage and output
> result better.
All the above done!!

>
> in src/HttpRequest.h:
> - spelling... s/avalile/avaliable/ s/reuest/request /
> - please write these as doxygen comment syntax (start with '///< ...'
> when after the member variable definition, '/** ... */' when before)
> - please write comments that describes the member/variable in easily
> understood terms. You can omit repeating details easily understood
> from the names sometimes, as is the case for storeId() but ..
>
> - for store_id it is not clear from the name what it is exactly.
> "Storage of StoreID for the specific cases that the request not
> avalile" leaves me just as much in the dark about what this variable
> holds.
> + Prefer something like: "The ID string used internally by Squid to
> uniquely de-duplicate this requests URL with other URLs stored objects."
>
> - for storeId() soemthign similar with also a statement also about
> whether the result char* is a nul-terminated string or not (if not
> please return a StringArea object instead of char*.) A statement to
> the effect that it always returns a string and never NULL would also
> be useful.
>
>
Well I have tried and hope the current doc is good as a description.

> in src/cache_cf.cc:
> - we only indent with 4 spaces. The requirePathnameExists() is using
> something like 8. Same applies elsewhere.
>
   Fixed.
>
> in src/cf.data.pre:
> - under store_id_program
> + on "ERR" in protocol description, please add a statement:
> "The default is to use HTTP request URL as the store ID"
>
> + the "WARNING:" paragraph is specific to URL-rewriter screwups. The
> storeID does need a WARNING as well, but the paragraph there should
> describe the known problems with pointing two URLs at one stored
> object (risk of getting the de-dup wrong, client visible when problems
> oc cur, etc).
>
OK.
I added note about refresh_pattern but I am not sure if it's the best
place here.

> - under store_id_children
> + s/backlog of URLs/backlog of requests/
> + the final paragraph uses the term "request ID" when talking about
> channel-ID field. Please copy the wording change made here:
> http://maststore_id_children
> er.squid-cache.org/Versions/v3/3.HEAD/changesets/squid-3-12505.patch
>
DONE!

> - under store_id_bypass, I think we should turn bypass for this
> helper ON by default. While that results in cache object duplication
> it prevents a Squid dying with FATAL if the helper is badly written or
> simply can't stand the load. IMO a little cache duplication is
> reasonable price to pay for better uptime. Helpers which can take the
> load will not trigger the bypass.
You convinced me!
When concurrency will be out of bugs most helpers can be used with it
and also will allow more space.

>
> in src/client_side.cc:
> - the only change is a useless whitespace removal. Please undo that.
>
DONE!
> in src/client_side_reply.cc:
> - your question about 'rawBug' is no. The char* is passed down
> through multiple layers of code some of which uses it unconditionally
> as if it was a nul-terminated string. You need to use termedBuf()
> instead to ensure they continue to work after StringNG happens.
>
Thanks
> - you are also using the pattern "http->store_id.size() BLAH ?
> http->store_id.termedBuf() : http->uri" in various places with
> inconsistent BLAH (!=0, <0, >0)
> + please use the inline accessor method http->storeId() instead. You
> may need to duplicate the HttpRequest::storeId() method in
> clientReplyContext. OR convert the reply code to using
> http->request->storeId()
>
FIXME: I have considered using another method but since it was
implemented before with the variable I used the more direct approach to
the variable instead of creating a method.

> + please update that method to perform the above pattern in a
> suitable form.
>
If we do want to force basic store_id *size* I will implement a method
with validation otherwise I dont think it's needed.

> - in clientReplyContext::doGetMoreData I do not quite understand your
> new documentation NOTE:
> + s/relpy/reply/
> + what is a ' reply of "HTTP/" ' and how does it occur at this point
> in the code? what is the problem you hint at without stating?
> + what is going on with debugs 'section'? no debugs() statement
> should ever change the run-time state by being run/displayed. Yours
> does not change any state, so how does is affect anything?
>
A FIXME on that in the code.
>
> in src/client_side_request.cc:
> - documentation needs to start with /** instead of /* for doxygen
> format. Several places.
> - clientStoreIdAccessCheckDone
> + please use static_cast in new code casting ...
> "ClientRequestContext *context = static_cast<ClientRequestContext
> *>(data);"
> + "The nilReply is marked as Unknown if Will not set as Error." is
> not needed with the method documentation saying the same thing but
> better.
>

> - ClientRequestContext::clientStoreIdStart()
> + "The methods starts the helper chain." is very vague. Prefer
> something like:
> "Start locating an alternative storeage ID string (if any) from
> admin configured helper program. This is an asynchronous operation
> terminating in ClientRequestContext::clientStoreIdDone() when completed."
>
Changed to your words since I must admit I dont have better words then
the suggested.

Sorry for the delay.

Eliezer