Re: [PATCH] HTTP/1.1 response caching upgrade

On 18.10.2012 04:42, Alex Rousskov wrote:
> On 10/16/2012 03:24 AM, Amos Jeffries wrote:
>> Updated patch with requested changes.
>
> Looks OK to me although I have not verified each individual CC
> handling
> line.
>
>> + if ((mayStore |= rep->cache_control->Public())) {
>> + } else if ((mayStore |=
>> (rep->cache_control->mustRevalidate() &&
>> !REFRESH_OVERRIDE(ignore_must_revalidate)) )) {
>> + } else if ((mayStore |= (rep->cache_control->noCache() &&
>> !REFRESH_OVERRIDE(ignore_no_cache)) )) {
>> + } else if ((mayStore |= rep->cache_control->sMaxAge())) {
>> + }
>
> If mayStore starts as false and is modified only once, I would
> replace
> "|=" cleverness with a simple assignment.

Okay. Done.

>
>> + debugs(22, 3, HERE << "YES because Authenticated and
>> server reply Cache-Control:public");
>> + debugs(22, 3, HERE << "YES because Authenticated and
>> server reply Cache-Control:public");
>> + debugs(22, 3, HERE << "YES because Authenticated and
>> server reply Cache-Control:no-cache");
>> + debugs(22, 3, HERE << "YES because Authenticated and
>> server reply Cache-Control:s-maxage");
>
> These should not be "YES" because we may end up returning 0 or -1
> later
> anyway. We are already using "MAYBE" for -1 case. I suggest replacing
> "YES because Authenticated and ..." text with "Authenticated but ..."
> text.

Sure. Done.

>
> These polishing touches do not require another round of review as far
> as
> I am concerned.
>
> Do you want us to run this by Co-Advisor before the commit?

 From the wiki coadvisor checklist I noticed the auth and pragma cases
are very under-represented and I'm only expecting 3-5 cases to change
from #! or V from this despite the great leap in cacheable content
(which is more based on prevalence of no-cache). If it's not going to
take long that would be great. I'll hold this until tomorrow then apply
and release 3.2.3 and 3.3.0.1 sometime over the weekend.

Amos
Received on Thu Oct 18 2012 - 00:07:01 MDT