Re: [PATCH] StoreID latest implementation in sync with rev 12552. stage 2-3 from 3.

On 29/12/2012 8:16 p.m., Eliezer Croitoru wrote:
> Thanks for all the help until now.
>
> This PATCH is in sync with trunk which comes after the last preview.
> I tried my best until now in the small windows of time I have for the
> project with hopes for the best.
>
> This no longer a PREVIEW since most of the code was reviewed couple
> times and was tested in a small production testing environment for
> couple weeks with a great success.
> I left couple FIXME and TODO in the code.
>
> One question I had regarding the documentation\comments:
> I have seen in the code couple places where the first line of a
> comment was out-of-line from the comment start like "/**\n * text\n
> */" and others where the first test is inline with comment start and
> wasn't sure how much it's important.
> I have tried to use in most of my code the "/**\n" since I think it's
> more readable.
> Is there a preference about it?

Up to you. Text on the first line is used as a brief description when
the full text may be long.

>
> I have decided to leave the store_id string variable null in cases
> it's not needed which allow to use the store_id.undefined().
> This adds the flexibility of a basic true\false check when needed later.

Okay.

>
> While testing some helpers I have seen a bug when using concurrency
> which I will debug later after committing the working StoreID to trunk.
>
> TESTS: I tested this patch and found some bugs related to helper code
> and I am looking for other tests.
> cases:
> - regular ok reply with store-id=value(not empty) = works.
> - reply with empty store-id key pair = getting some assertion problem.
> - ok reply with the original url = works and wont set the store_id
> value.
> - simple PURGE request from squidclient = erases the object.
>
> havn't tested yet?
> - broker helper replies(drop me some if you have in mind).
> - reply with k-v other then store-id.
> - reply with too many spaces between parts.
> - htcp related tests.
> - icp related tests.
>
>
> Regarding back-porting the code into 3.2 branch:
> What do think? There is quite a different in the design of the code
> in 3.2 vs trunk.

I am probably releasing 3.3 as stable next month. In which case it will
not be worth the effort.

>
> The next thing after this patch is adding ICAP the option to respond
> with some StoreID for a request.
> How do I even start on that?
> and a case I had in mind is what to do when Store-ID is used in ICAP?
> do we allow second helper?
> if so what and how do we send to the helper since it comes after the
> ICAP REQMOD service ?

Logically both are working on whatever the storeId currently is to
produce no-change or a new one. So I think we should let the helper be
passed the storeID from ICAP. Leaving it up to the admin whether to use
two or one, and when each operates.

>
> I have reviewed the patch and code couple times but I still might not
> see something.
>
> If someone have some scripts of basics code\syntax checks I will be
> more then happy to have them in order to check any future code related
> the project.

./scripts/sourcemaintenance.sh is the main one. It requires a secific
version of astyle to do full syntax checks but some checks are always run.

Amos
Received on Sat Dec 29 2012 - 08:26:04 MST