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

On 12/21/2012 7:36 AM, Amos Jeffries wrote:
> On 21/12/2012 2:20 p.m., Eliezer Croitoru wrote:
>> 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.
>
>>> 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.
>
> I think it is the right place for that. refresh_attern itself is not the
> problem, but only when storeId functionality is enabled. So people
> looking at storeId directive need to be told. If you wish to add a not
> to refresh_pattern *as well* that would be fine.
>
>>> - 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.
>
> Not sure what you mean by that, but will review the new patch and see if
> it makes sense.
>
What I meant was that since before my code implementation the code was:
const char *url = http->uri;

I will use the "x ? x:y" logic with the new variable but I kind of
understand it would be too much.

>>
>>> + 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.
>
> I was basing that on what you are currently doing, using size(). String
> only provides Size() and defined(). StoreId needs to be a >0 length
> string to be useful - defined() will return true for 0-length strings
> which have been defined as "" and not very useful for storeId.
>
> I'm not sure if we want to enforce a minimum length key or just warn
> loudly for short keys. That is a test for the helepr reply handler to do
> though.
>
> I was just thinking you could add:
> inline const char* ClientReplyContext::storeId() { return
> (http->store_id.size() > 0 ? http->store_id.termedBuf() : http->uri); }
>
> (or some equivalent using urlCanonical() instead of http->uri.)
>
we cant use urlCanonical since it's based on the http->request which is
not available in most cases http->store_id\http->uri selection is required.
The "size > 0" should be enforced in the helper reply process so no real
need for that here.
I have tried to put something in the "case HelperReply::Okay:" which
seems OK to me.

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

I hope till the end of next week I will have my code in sync with trunk
and tested.

Eliezer
Received on Fri Dec 21 2012 - 08:09:25 MST