Sarah Jelinek has posted comments on this change. Change subject: Persistent cache support for NVM ......................................................................
Patch Set 21: (19 comments) Builds and tests successfully. Still some comments from the tidy bot which I don't think I can resolve. 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 (access(PmemPath.c_str(), F_OK != 0)) { > But this means that you get cross-testcase pollution, which is a unit testi I fixed this. I now have it use the default location as set by the test, and then close the pool after element insertion. Repoen and verify the pool contents. This isn't the most ideal way to do this however, until we have a way to inject errors to force the possibility of partial updates to the pmem pool this is what I have available to me. Failing on allocation doesn't provide us with error injection that helps verify the on-media data. What we need is an injector that forces errors at random intervals. Line 76: } > There's not any guarantee that the tests all run in the order they are decl Fixed. I close the cache, then reopen it which rebuilds it. Then I verify the contents via a lookup. http://gerrit.cloudera.org:8080/#/c/2593/15/src/kudu/util/nvm_cache.cc File src/kudu/util/nvm_cache.cc: Line 846: TOID_TYPE_NUM(struct KeyVal), PMEM_ALLOC, > I think we need _some_ form of upgrade, even if it's something as simple as Implemented a major/minor version set/check. The pool, upon creation, has major/minor values as set in the nvm_cache.cc file. If the pool is opened and the on media version is not the version specified in the nvm_cache.cc #defines, the open fails. The change in major/minor versions implies a change to the on-media format. Therefore, it's up to the kudu developers to manage this an update the version values as required. http://gerrit.cloudera.org:8080/#/c/2593/18/src/kudu/util/nvm_cache.cc File src/kudu/util/nvm_cache.cc: Line 55: // what constitues an incompatible change and if the pool can be > isn't this used in both volatile and persistent mode? Yes. I will update the comment. Fixed. Line 60: DEFINE_string(nvm_cache_path, "/pmem", > is this a file that will be created within nvm_cache_path? Yes. I will update comment to reflect this. Fixed. Line 100: class MetricEntity; > I'm not following this. The members above are 4 bytes each, so after this p Yes, you are right. You wanted this structure to be aligned on an 8 byte boundary. It is without the padding, as it was originally written: struct KeyVal { uint64_t key_len; uint64_t value_len; // Really a boolean but for alignment reasons make it uint8_t. // This member is set at the very end prior to persisting the KeyVal // object. This means that in the case of an interruption in service // the pmemobj object is not considered complete. // If 'valid' is not set then upon restart this entry is discarded. uint8_t valid; uint8_t kv_data[]; // holds key and value data }; The first 2 members are of course aligned on 8 bytes. The compiler is going to add a pad of 7 bytes after the valid member. The key/value array, even though it's dynamic should align to 8 bytes since the largest member size is 8bytes. Am I missing what you are asking for here? Line 105: enum AllocType { > typo: upon fixed. Line 131: int cache_minor; > hrm, still not really following. the offset is relative to what? The offset in this case is the offset of the kv_data member within the struct KeyVal. I am going to modify this comment to make it more general as it can be used to find any Object ID (OID) within the pool based on the structure member and structure type. In this case I use it to find the Object ID of the containing struct KeyVal so I can find the OID within the pool. I do this by passing in the address of the kv_data instance I have and the offset of this member(kv_data) within struct KeyVal.I have wrapped this in a function simply to make it cleaner. Does this help? PS18, Line 152: kv = static_cas > in the current implementation, it's not always a variable-length struct, ri Yes, you are correct. I changed this to reflect this. PS18, Line 159: he str > typo fixed. Line 277: return ptr; > should not be 'explicit' since it has multiple params fixed. Line 298: } > still missing docs Fixed. Line 479: if (deep && e->repopulated != 0) { > this can be defined with tighter scope (inside the loop below) right? What it is you want defined within the scope of the loop? The way I have it implemented here these are defined only once and then used within the loop. I would assume this is what we want to do. Line 493: bool NvmLRUCache::Unref(LRUHandle* e) { > can we do this outside the lock to avoid contention? Yes, we can. I will move it. Line 494: DCHECK_GT(ANNOTATE_UNPROTECTED_READ(e->refs), 0); > under what circumstances would you get the same entry twice? We couldn't. This was a leftover(I think) from when I originally freed all the entries outside the loop. I will remove. Fixed. Line 596: NvmLRU_Append(e); > in the case of last_reference, this means that the entry is no longer in th Fixed Line 674: PopulateCacheHandle(e, eviction_callback); > use unique_ptr now I removed this. It wasn't required. Line 701: 0 : Bits::Log2Ceiling(base::NumCPUs()); > this is the default Actually, I don't need to do this at all. I removed it. Line 859: handle->charge = charge + key_len; > - should use JoinPathSegments here, so that if someone doesn't add a traili Fixed. -- 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: 21 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sarah Jelinek <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sarah Jelinek <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
