Todd Lipcon has posted comments on this change.
Change subject: Persistent cache support for NVM
Patch Set 18:
Line 74: if
> 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.
> 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.
Line 55: "This flag is used for volatile mode. It must be an
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; // 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.
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 http://gerrit.cloudera.org:8080/2593
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Owner: Sarah Jelinek <sarah.jeli...@intel.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sarah Jelinek <sarah.jeli...@intel.com>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>