Re: refresh.cc - refreshIsCachable()

On 27 Apr 2006, at 11:10, Duane Wessels wrote:

>
>
>
> On Tue, 25 Apr 2006, Doug Dixon wrote:
>
>> It looks like the call to refreshCheck() is only used to short-
>> circuit the function (if you can call it a short circuit, given
>> the size of the callout) and to update some statistics.
>
> The main, and original, purpose of refreshCheck() is to check the
> request
> against the 'refresh_pattern' configuration values from squid.conf.

Yep, refreshCheck() is used elsewhere and that's fine - it's just the
call from refreshIsCachable() that I'm questioning.

>
>> I can't see the reason stats being used anywhere, but maybe I
>> overlooked something. (And anyway... should this function even be
>> updating the refreshCounts[rcStore].total stat, given the fact
>> this just checks a couple of flags?)
>
> The 'reason' return value and related statistcs are definately used.
> You can see refresh statistics in the cache manager 'refresh' page.
>

Ah sorry, you're right :)

>> However, the return value can be calculated without calling it,
>> and the statistic it updates is never used. Would something like
>> this be cleaner/quicker and still correct?
>
> I'm not sure which function "it" refers to.
>
> We can't NOT call refreshCheck(), ala your suggested
> refreshIsCachable()
> replacement. refreshIsCachable() could be made slightly shorter and
> cleaner, but I don't think its worth the trouble.

The return value of refreshIsCachable() can be calculated without
making a call to refreshCheck().

I.e. you can remove the call to refreshCheck() from refreshIsCachable
(), and refreshIsCachable() will still return the correct result.

The only side-effect is the loss of the refreshCounts[rcStore]
stats... but since the meaning of this stat is (per CacheMgr cgi)
"calls to refresh check", this stat should always be zero if we don't
make the call. I.e. if we lose the call to refreshCheck, the right
thing to do is lose the statistic.

Cheers
Doug
Received on Thu Apr 27 2006 - 10:36:14 MDT