Adar Dembo has posted comments on this change. ( )

Change subject: KUDU-2920: Block cache capacity validator shouldn't run on an 
NVM block cache

Patch Set 2:


The ASAN failure is a known flaky test (KUDU-1736) but the IWYU failure is real 
and should be addressed.
Commit Message:
PS2, Line 7: KUDU-2920: Block cache capacity validator shouldn't run on an NVM 
           : cache
Should limit the commit summary to just one line.
File src/kudu/cfile/
PS2, Line 92: // Validates the block cache capacity won't permit the cache to 
grow large enough
            : // to cause pernicious flushing behavior. See KUDU-2318.
Now that the free function is visible beyond its TU, move this comment to the 
PS2, Line 98:   if (FLAGS_block_cache_type == "DRAM") {
Nit: we generally style Kudu code to reduce nesting as much as possible, to 
improve code readability. So in this case, you could rewrite it like so:

  if (FLAGS_block_cache_type != "DRAM") {
    return true;
  <capacity checks>
  return true;
File src/kudu/cfile/
PS2, Line 1054:   bool save_force_block_cache_capacity = 
              :   int64 save_block_cache_capacity_mb = 
You don't need to manually save/restore gflag values; every test fixture that 
inherits from KuduTest will do so automatically. See test_util.{cc,h} and 
google::FlagSaver (from gflags).
PS2, Line 1059:   if (GetParam() == Cache::MemoryType::NVM) {
              :     ASSERT_TRUE(kudu::cfile::ValidateBlockCacheCapacity());
              :   }
              :   if (GetParam() == Cache::MemoryType::DRAM) {
              :     ASSERT_FALSE(kudu::cfile::ValidateBlockCacheCapacity());
              :   }
Nit: maybe clearer as a switch statement? If not, at least use "if ... else if 
... ". Or, since these are the only two values for MemoryType, "if ... else 
CHECK_EQ(GetParam() == ...) ...".

To view, visit
To unsubscribe, visit

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f50e9dd901280b5c32576e43165292299922995
Gerrit-Change-Number: 14212
Gerrit-PatchSet: 2
Gerrit-Owner: Volodymyr Verovkin <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 11 Sep 2019 16:42:08 +0000
Gerrit-HasComments: Yes

Reply via email to