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

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.

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

>
>> - 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
Received on Fri Dec 21 2012 - 05:36:27 MST