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

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".

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

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.

> === 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?

> 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.
Received on Tue Feb 05 2013 - 19:15:00 MST