Re: [PATCH] rock fixes and improvements

On 04/28/2014 09:11 AM, Amos Jeffries wrote:

> in DiskIO/IpcIo/IpcIoFile.cc:
> * please use SBuf for DbName global

Done.

> * please remove HERE in chunk at line 888:
> - debugs(79,3, HERE << "rock db opened " ...

Done.

> in MemStore.cc:
> * consider making ExtrasLabel and SpaceLabel into SBuf. same in
> Transients.cc

Considered and even tried! Rejected because the shm_new/old() API needs
c-string, *Label globals should be constant, and SBuf::c_str() is not
constant. We can make it work, but the changed code just looks ugly and
"confused".

What actually needs to be done here is converting shm_new/old() API to
use SBuf, but that is a non-trivial change outside this work scope
(primarily because it triggers a cascading effect with changes to other
low-level APIs). Quality patches welcomed.

> in Store.h:
> * new documentation for checkCacheable() is unclear. It only made sense
> after reading the related functions descriptions and some code.
> - Consider "whether generally cacheable" as the brief and expanding the
> brackets to separate sentences.
> - please consider making the function return bool for even better

Here is the revised declaration:

> /// Satisfies cachability requirements shared among disk and RAM caches.
> /// Encapsulates common checks of mayStartSwapOut() and memoryCachable().
> /// TODO: Rename and make private so only those two methods can call this.
> bool checkCachable();

Is it acceptable?

I changed the return type from int to bool per your request, but the
method implementation still uses 0/1 (instead of true/false) to avoid
widespread non-changes obscuring important/sensitive code adjustments
done in the patch. The 0/1s can be converted in a separate commit.

> * please use "// TODO" (non-doxygen slashes) for private TODOs, or
> proper doxygen "/// \todo" tag for ones that to be published in the code
> manual TODO page.

Not done per IRC discussion: We need a better understanding of what
markup to use for TODOs and XXXs, where to use it, and when.

> in src/ipc/StoreMap.cc:
> * Missing decrement?
> @@ -64,7 +74,7 @@
> - --shared->count;
> + anchors->count;

Fixed. Good catch!

Revised patch is attached (same caveats; no other changes). I also
attached a diff showing incremental changes specific to this review.

Thank you,

Alex.