Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14212 )

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


Patch Set 2:

(5 comments)

The ASAN failure is a known flaky test (KUDU-1736) but the IWYU failure is real 
and should be addressed.

http://gerrit.cloudera.org:8080/#/c/14212/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14212/2//COMMIT_MSG@7
PS2, Line 7: KUDU-2920: Block cache capacity validator shouldn't run on an NVM 
block
           : cache
Should limit the commit summary to just one line.


http://gerrit.cloudera.org:8080/#/c/14212/2/src/kudu/cfile/block_cache.cc
File src/kudu/cfile/block_cache.cc:

http://gerrit.cloudera.org:8080/#/c/14212/2/src/kudu/cfile/block_cache.cc@92
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 
header.


http://gerrit.cloudera.org:8080/#/c/14212/2/src/kudu/cfile/block_cache.cc@98
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;


http://gerrit.cloudera.org:8080/#/c/14212/2/src/kudu/cfile/cfile-test.cc
File src/kudu/cfile/cfile-test.cc:

http://gerrit.cloudera.org:8080/#/c/14212/2/src/kudu/cfile/cfile-test.cc@1054
PS2, Line 1054:   bool save_force_block_cache_capacity = 
FLAGS_force_block_cache_capacity;
              :   int64 save_block_cache_capacity_mb = 
FLAGS_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).


http://gerrit.cloudera.org:8080/#/c/14212/2/src/kudu/cfile/cfile-test.cc@1059
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 http://gerrit.cloudera.org:8080/14212
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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

Reply via email to