Todd Lipcon has posted comments on this change. Change subject: Persistent cache support for NVM ......................................................................
Patch Set 18: (19 comments) http://gerrit.cloudera.org:8080/#/c/2593/15/src/kudu/util/nvm_cache-test.cc File src/kudu/util/nvm_cache-test.cc: 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 antipattern. 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. http://gerrit.cloudera.org:8080/#/c/2593/15/src/kudu/util/nvm_cache.cc File src/kudu/util/nvm_cache.cc: 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. http://gerrit.cloudera.org:8080/#/c/2593/18/src/kudu/util/nvm_cache.cc File src/kudu/util/nvm_cache.cc: 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 typo 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* eviction_callback, 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-MessageType: comment Gerrit-Change-Id: Id72570ead662670bf42175756a18ae08d7cd0a07 Gerrit-PatchSet: 18 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sarah Jelinek <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sarah Jelinek <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
