Re: [PATCH] Bug 3686 - refactor the config options controlling max object size

On 6/02/2013 8:14 a.m., Alex Rousskov wrote:
> On 02/05/2013 03:36 AM, Amos Jeffries wrote:
>> http://bugs.squid-cache.org/show_bug.cgi?id=3686
>> + if (static_cast<uint64_t>(newMax) > maxSize()) {
>> + debugs(47, DBG_PARSE_NOTE(2), "WARNING: object 'max-size' option for " << path << " cannot exceed " << maxSize());
>> + max_objsize = maxSize();
>> + return;
>> + }
> Please adjust this to say "cannot exceed total cache_dir size " or
> similar to clarify where the limit is coming from.
>
> I would also say "ignoring max-size for <path> because it exceeds
> cache_dir <size>" instead of the "max-size cannot exceed ..." phrase, to
> tell the admin what Squid is doing instead of just telling them what
> Squid cannot do. This will make it clear that their max-size option has
> no effect.
>
> In summary:
>
> debugs(47, DBG_PARSE_NOTE(2), "WARNING: ignoring max-size for " <<
> path << " because it exceeds total cache_dir size " << maxSize());
>
>
>> + /// the minimum size of singel object which may be stored here
> "single" typo
>
> I would rephrase as "smaller objects will not be added and may be purged".
>

Actually this was completely wrong anyway. minSize() *is* the function
paired with maxSize() and seems to be related to the low watermark somehow.
Dropped the documentation change.

>
>> - virtual int64_t maxObjectSize() const { return max_objsize; }
>> + /// The maximum size of single object which may be stored here.
>> + int64_t maxObjectSize() const;
> Same rephrasing suggestion as the above.
>
> Please do not remove "virtual". It is a useful reminder that this is a
> part of the Store API.

Why add virtual table entries? this is not a replaceable function any
more. This is an accessor method for the SwapDir base object.

>
> Also, I think it would be appropriate (and useful) to document maxSize()
> method in this patch because it looks symmetrical to now-documented
> minSize() but has a very different meaning.

N/A now and I'm not sure I understand it well enough to add documentation.

>
>> === modified file 'src/peer_digest.cc'
>> --- src/peer_digest.cc 2013-01-30 15:39:37 +0000
>> +++ src/peer_digest.cc 2013-02-03 13:38:13 +0000
>> @@ -347,6 +347,7 @@
>>
>> req->header.putStr(HDR_ACCEPT, "text/html");
>>
>> + // XXX: this is broken when login=PASS, login=PASSTHRU, login=PROXYPASS, login=NEGOTIATE, and login=*:...
>> if (p->login)
>> xstrncpy(req->login, p->login, MAX_LOGIN_SZ);
>>
> An unrelated change?

Er. yes. dropped.

>
>> StoreEntry *const pe = addEntry(i);
>>
>> + printf("pe->swap_status == %d (SWAPOUT_WRITING=%d)\n", pe->swap_status, SWAPOUT_WRITING);
>> +
>> CPPUNIT_ASSERT(pe->swap_status == SWAPOUT_WRITING);
>> CPPUNIT_ASSERT(pe->swap_dirn == 0);
>> CPPUNIT_ASSERT(pe->swap_filen >= 0);
>
> Perhaps remove the added debugging to reduce "make check" noise? Or make
> it conditional on swap_status being not SWAPOUT_WRITING?
>
>
>
>> + rep->setHeaders(HTTP_OK, "dummy test object", "x-squid-internal/test", 0, -1, squid_curtime + 100000);
> You may want to add a line to the commit message to note why this change
> was necessary (and that test cases now have weak/indirect checks for
> max-size code).
>
>
> All of the above can be done during commit, without another review cycle
> IMO.
>
>
> Thank you,
>
> Alex.
>

Applie dto trunk as rev.12662.

Amos
Received on Sat Feb 09 2013 - 00:55:01 MST