Sarah Jelinek has posted comments on this change. Change subject: Add implementation of the block cache on NVM running in persistent mode. ......................................................................
Patch Set 8: (6 comments) Addressed all comments. Will push changes today. http://gerrit.cloudera.org:8080/#/c/1347/2/CMakeLists.txt File CMakeLists.txt: Line 755: DEPS protobuf) > No, we link with shared or static based on what type of build we are doing. Actually, changed it to PMEMOBJ_DEPS. http://gerrit.cloudera.org:8080/#/c/1347/2/cmake_modules/FindPmem.cmake File cmake_modules/FindPmem.cmake: Line 47: set(PMEMOBJ_DEPS pmem) > probably just name this PMEMOBJ_DEPS ok. changed. Line 69: PMEMOBJ_DEPS > this should probably be PMEMOBJ_DEPS? Yes. fixed. http://gerrit.cloudera.org:8080/#/c/1347/2/src/kudu/util/cache-test.cc File src/kudu/util/cache-test.cc: Line 72: if (GetParam() == DRAM_CACHE) { > Ok. I saw this used this way in the gflags_unittest code so thought it was I changed this to: #if defined(__linux__) if (google::GetCommandLineFlagInfoOrDie("nvm_cache_path").is_default) { FLAGS_nvm_cache_path = GetTestPath("nvm-cache"); is_default = true; ASSERT_OK(Env::Default()->CreateDir(FLAGS_nvm_cache_path)); } #endif switch (GetParam()) { case DRAM_CACHE: break; #if defined(__linux__) case NVM_CACHE: FLAGS_nvm_cache_persistent = false; break; ... Like we do in cfile-test.cc. Line 92: uint8_t* key_val = cache_->Allocate(key_str.size(), val_str.size()); > - nit: "mmapped" rather than "memmapped" (or "memory-mapped") The issue is that customers may want to name their own pools. So, the only way I could do what you are suggesting is to have yet another flag, --nvm_pool_name, or something that defaults to "pm" unless otherwise set. I agree it's hard to document and configure in these tests. I don't like it either. Are you open to adding another flag? This is just the way nvml works with regard to vmem and pmem pools. http://gerrit.cloudera.org:8080/#/c/1347/2/src/kudu/util/nvm_cache.cc File src/kudu/util/nvm_cache.cc: Line 252: h = next; > is there a typedef for this in pmemobj.h? Perhaps we can typedef it at the No, there is no typedef for this. So, leaving as is. -- To view, visit http://gerrit.cloudera.org:8080/1347 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2dcf474bce9f2375b0ddde38312db7d82a81835c Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sarah Jelinek <sarah.jeli...@intel.com> Gerrit-Reviewer: Anonymous Coward #90 Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sarah Jelinek <sarah.jeli...@intel.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
