Re: [PREVIEW] Large Rock and Collapsed Forwarding

On 3/07/2013 11:05 a.m., Alex Rousskov wrote:
> Hello,
>
> The attached patch implements Large Rock and Collapsed Forwarding
> support. The following Launchpad branch contains detailed commit
> messages, most of which would be good for merging back into trunk:
>
> This email summarizes what works and what does not. What works in our
> lab tests:
>
> * Large Rock support (caching of arbitrary large files using rock
> cache_dirs).
>
> * Collapsed Forwarding support when the response is [eventually] cached
> in memory.
>
>
> What has not been tested and probably needs more work or at least polishing:
>
> * Collapsed Forwarding support when the response is [eventually] cached
> on disk.
>
> * 32-bit OS support (there is one 64bit Atomic -- swap_file_sz).
>
> * I need to check that all new declarations have descriptions and resync
> with trunk.
>
> I do not expect significant changes going forward, but I am
> uncomfortable calling this a "[PATCH]" at this time because of the above
> caveats.
>
> The above features were initially tested in non-SMP mode (and worked
> OK), but all recent work and tests were focusing on SMP configurations.
>
>
> HTH,
>
> Alex.

* Please remove HERE macro from all the new code debugs().

in MemBlob.*
* this API change could go in immediately and shrink the branch diff a
little.

in StoreEntry
* Since you are changing the API to StoreEntry::lock()/unlock() can you
please take the opportunity to migrate them both to different method
names and use the base/Lock.h API for the background counting mechanics.
That will allow proper ref-counting Pointer to cleanup more of those
places we are using nasty "protect-our-feets" lock hacks like that bug
3480 which you point out in the store comments.
  - this API change could also go in early and shrink the branch diff a
fair bit.

in CollapsedForwarding.cc:
* please sort the #include lines alphabetically as per the guidelines
* the TODO "add proper message type?" seems to already be done.

in src/store_dir.cc
* adding empty line at chunk ~818

in Transients.cc:
* #include ordering again.
* there are several occurances of double-empty lines in this file (maybe
elsewhere in the new code too).
* unless there is a reason to use signed <0 values for
Transients::readers [and StoreController::transientReaders] please make
their return values an unsigned type.
* please document why Transients::maintain() is an empty function. Same
for several of the other no-op members.
* why should Transients::search() result in fatal()? surely it would
allow more consistent storage area searching if it were another no-op
function and/or Transients could be treated like a cache (at least for
lookups).
* why is Transients::EntryLimit setting a fixed 16K limit on concurrent
requests? surely this should be the max_filedescriptors limit to handle
the case where 100% of FD are consumed by client requests?
* please be consistent with the Squid style of function definitions
having return type on the line above function name. The TransientsRr
members are all using the more normal definition style.

in src/Transients.h:
* the terminal #endif comment is not matching the initial file wrapper
definition.
* is Transients class really about cache HIT? surely it is about MISS or
REFRESH which are not yet fully cached.

in src/client_side_reply.cc
* please take advantage of touching the debugs() in
clientReplyContext::identifyStoreObject to remove incorrect
"clientProcessRequest2:" and HERE in the statements altered.
* regarding "TODO: why is !.needValidation required here?"
  - because must-revalidate and other equivalent transactions MUST be
passed to the origin for validation on every client request. We are not
permitted to collapse them.

in src/fs/rock/RockForward.h:
* after updating to the latest trunk you can safely call this file
forward.h like the rest of the forward declaration files in Squid.
* it has some double-empty lines to remove as well

in src/fs/rock/RockIoState.cc:
* s/diring/during/
* please format the documentation for Rock::IoState::tryWrite a bit
cleaner with "*" as per the rest of the code block comments. Remembering
that the line starting "/**" if containing text will be used in
isolation as a one-liner brief description.
* Rock::IoState::writeToBuffer can use \return please.

in src/fs/rock/RockRebuild.cc
* why do you seem to be removing maxObjectSize() from Rock store? That
max-size option should remain even if you are moving to slot-size for
slot management. If you are retaining maxObjectSize() and I missed it,
why are the rock store stats dump removing it? AFAICT that function
should dump both slot-size and max-size details.

* contains a block of bulk documentation which appears to be for the
Rebuild constructor but is separated by two empty lines so I'm going to
assume its not intended to be that way. Please consider formatting it as
a doxygen module page (search for "defgroup" under src/ for examples) -
prefix this block with:
"
/**
  * \defgroup RockFsRebuild Rock Store Rebuild
  * \ingroup Filesystems
  *
"
... and use \section (new sub-section) and \par (new paragraph) as
necessary for the main textual formatting.

I only did a brief scan over and got as far as Rock. May have time for
more later.

I'm also seeing a lot of formating issues. Do you have astyle 1.23 to
run the scripts/source-maintenance.sh over the branch?

Amos
Received on Wed Jul 03 2013 - 12:36:46 MDT