Alexey Serbin 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) it seems IWYU is not happy: http://jenkins.kudu.apache.org/job/kudu-gerrit/18715/BUILD_TYPE=IWYU/artifact/build/latest/test-logs/iwyu.log IWYU doesn't produce meaningful results on macOS, but you can always run it on Linux build machine. http://gerrit.cloudera.org:8080/#/c/14212/2/src/kudu/cfile/block_cache.h File src/kudu/cfile/block_cache.h: http://gerrit.cloudera.org:8080/#/c/14212/2/src/kudu/cfile/block_cache.h@224 PS2, Line 224: bool ValidateBlockCacheCapacity(); Add a line of documentation for this function. Also, maybe it's possible to get the validator by name and not exposing this? Exposing the function just for the testing purpose doesn't look too good to me. 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@98 PS2, Line 98: if (FLAGS_block_cache_type == "DRAM") { I think from the point of readability it might be better to have: if (FLAGS_block_cache_type !== "DRAM") { return true; } ... the rest of the DRAM-specific validation code ... 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; This is not needed: TestCFileBothCacheMemoryTypes inherits from KuduTest, where google::FlagSaver takes care of that automatically: https://github.com/apache/kudu/blob/148a0c7bec6554724339a2235cbd723fb74be339/src/kudu/util/test_util.h#L74 http://gerrit.cloudera.org:8080/#/c/14212/2/src/kudu/cfile/cfile-test.cc@1058 PS2, Line 1058: FLAGS_block_cache_capacity_mb = 1000000; Is it possible to somehow make it working as expected even if this test is run on a machine with 256GB of memory? http://gerrit.cloudera.org:8080/#/c/14212/2/src/kudu/cfile/cfile-test.cc@1066 PS2, Line 1066: FLAGS_block_cache_capacity_mb = save_block_cache_capacity_mb; : FLAGS_force_block_cache_capacity = save_force_block_cache_capacity; ditto: FlagSaver does that automatically, and even if any of the asserts triggers during the test scenario -- 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:50:31 +0000 Gerrit-HasComments: Yes
