Sarah Jelinek has posted comments on this change. Change subject: Persistent cache support for NVM ......................................................................
Patch Set 11: (4 comments) Initial review. http://gerrit.cloudera.org:8080/#/c/2593/11/src/kudu/util/nvm_cache.cc File src/kudu/util/nvm_cache.cc: Line 14: // We use this API to implement a cache which treats persistent memory or I think this comment isn't correct. I would say: 'We use this API to implement a cache which makes use of non-volatile memory either as DRAM or as a persistent cache. Line 164: uint8_t* kv_data; // Either pointer to pmem or space for volatile pmem. This comment doesn't make sense. I wrote it so it's on me :-). I think this should simply say 'pointer to data on NVM media'. Or something like that. Line 308: // Wrapper around vmem allocation which injects failures based on a flag. Is this true? We do have the capability to inject failures but this function isn't just a wrapper for this. Perhaps we just state that this function allows for injecting failures. Line 463: l.unlock(); We do a FreeLRUEntries after this loop. -- 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: 11 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
