Todd Lipcon has posted comments on this change.

Change subject: Persistent cache support for NVM

Patch Set 18:

File src/kudu/util/

Line 74:     if 
(google::GetCommandLineFlagInfoOrDie("nvm_cache_path").is_default) {
> Actually, the problem is that when a test is done the test path is no longe
But this means that you get cross-testcase pollution, which is a unit testing 

Line 76:       string PmemPath = FLAGS_nvm_cache_path;
> I don't think the test ordering is an issue here is it? This ordering is en
There's not any guarantee that the tests all run in the order they are 
declared. For example for some of our tests we shard them and run them 
distributed, so each machine only runs a subset of the cases. So, any 
cross-test dependency should be considered a bug.

If we want to test rebuilding a cache, we should explicitly destruct and 
re-construct the cache implementation, or use an external minicluster to build 
an integration test with persistence enabled, so that we can kill and restart 
servers at will.
File src/kudu/util/

Line 846: 
> We don't at this point. We need to talk about the version and upgrade strat
I think we need _some_ form of upgrade, even if it's something as simple as 
putting a 'header' object in the persistent area which includes a version 
number. New code can verify that the version number matches the current 
version, and bail out (or reset the cache) in the case that it doesn't match.

Otherwise, we'll have issues where people upgrade and then get spurious crashes 
due to data structure misalignment, etc, which is a support nightmare.
File src/kudu/util/

Line 55:               "This flag is used for volatile mode. It must be an 
existing directory.");
isn't this used in both volatile and persistent mode?

Line 60:               "when running in persistent mode."
is this a file that will be created within nvm_cache_path?

Line 100:   uint8_t   pad[4]; // Pad to align to 8 byte boundary
I'm not following this. The members above are 4 bytes each, so after this 
padding we should be at 12 bytes, no?

Line 105:   // If 'valid' is not set then upong restart this entry is discarded.
typo: upon

Line 131: // resides represented as a pointer to the object.
hrm, still not really following. the offset is relative to what?

PS18, Line 152: variable length
in the current implementation, it's not always a variable-length struct, right? 
In PMEM it's not variable-length because 'kv_data' is actually a pointer to the 
PMEM, whereas in VMEM 'kv_data' is the start of the data (rather than a pointer)

PS18, Line 159: ndlee 

Line 277:   explicit NvmLRUCache(PMEMobjpool* pop, PMEMoid root, VMEM* vmp);
should not be 'explicit' since it has multiple params

Line 298:   void PopulateCacheHandle(LRUHandle* e, Cache::EvictionCallback* 
still missing docs

Line 479:   LRUHandle* prev_remove_head = NULL; // Used for freeing each entry 
one at a time.
this can be defined with tighter scope (inside the loop below) right?

Line 493:         FreeEntry(to_remove_head, true);
can we do this outside the lock to avoid contention?

Line 494:       }
under what circumstances would you get the same entry twice?

Line 596:       FreeEntry(e, false);
in the case of last_reference, this means that the entry is no longer in the 
LRU and thus should actually be deep-freed

Line 674:   gscoped_ptr<EvictionCallback> eviction_callback_;
use unique_ptr now

Line 701:     eviction_callback_.reset(nullptr);
this is the default

Line 859:     string PmemPath = FLAGS_nvm_cache_path + FLAGS_nvm_cache_pool;
- should use JoinPathSegments here, so that if someone doesn't add a trailing 
'/' on nvm_cache_path, it won't make a bogus path.
- style: pmem_path, not PmemPath

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Id72570ead662670bf42175756a18ae08d7cd0a07
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sarah Jelinek <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sarah Jelinek <>
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to