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:


Addressed all comments. Will push changes today.
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.
File cmake_modules/FindPmem.cmake:

Line 47:   set(PMEMOBJ_DEPS pmem)
> probably just name this PMEMOBJ_DEPS
ok. changed.

> this should probably be PMEMOBJ_DEPS?
Yes. fixed.
File src/kudu/util/

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;
    switch (GetParam()) {
      case DRAM_CACHE:
#if defined(__linux__)
      case NVM_CACHE:
        FLAGS_nvm_cache_persistent = false;
Like we do in

Line 92:     uint8_t* key_val = cache_->Allocate(key_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.
File src/kudu/util/

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I2dcf474bce9f2375b0ddde38312db7d82a81835c
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sarah Jelinek <>
Gerrit-Reviewer: Anonymous Coward #90
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sarah Jelinek <>
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to