[kudu-CR] KUDU-2920: Block cache capacity validator shouldn't run on an NVM block cache
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: (1 comment) 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. As for finding the validator by name instead of exposing this function, if it's not easy or problematic to get the corresponding validator, I'm fine with keeping this as it is. -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 12 Sep 2019 18:10:54 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2920: Block cache capacity validator shouldn't run on an NVM block cache
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 = 100;
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 11 Sep 2019 16:50:31 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2920: Block cache capacity validator shouldn't run on an NVM block cache
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;
}
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 11 Sep 2019 16:42:08 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2920: Block cache capacity validator shouldn't run on an NVM block cache
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14212 to look at the new patch set (#2). Change subject: KUDU-2920: Block cache capacity validator shouldn't run on an NVM block cache .. KUDU-2920: Block cache capacity validator shouldn't run on an NVM block cache Validator should fail for options: --block_cache_type=DRAM --block_cache_capacity_mb=100 and succeed for options: --block_cache_type=NVM --block_cache_capacity_mb=100 --nvm_cache_path=/path/to/your/tmp/dir where 100 means "size of cache bigger than RAM" (1000GB) Change-Id: I9f50e9dd901280b5c32576e43165292299922995 --- M src/kudu/cfile/block_cache.cc M src/kudu/cfile/block_cache.h M src/kudu/cfile/cfile-test.cc 3 files changed, 41 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/14212/2 -- 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: newpatchset 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)
[kudu-CR] KUDU-2920: Block cache capacity validator shouldn't run on an NVM block cache
Volodymyr Verovkin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14212 Change subject: KUDU-2920: Block cache capacity validator shouldn't run on an NVM block cache .. KUDU-2920: Block cache capacity validator shouldn't run on an NVM block cache Validator should fail for options: --block_cache_type=DRAM --block_cache_capacity_mb=100 and succeed for options: --block_cache_type=NVM --block_cache_capacity_mb=100 --nvm_cache_path=/path/to/your/tmp/dir where 100 means "size of cache bigger than RAM" (1000GB) Change-Id: I9f50e9dd901280b5c32576e43165292299922995 --- M src/kudu/cfile/block_cache.cc M src/kudu/cfile/block_cache.h M src/kudu/cfile/cfile-test.cc 3 files changed, 43 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/14212/1 -- 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: newchange Gerrit-Change-Id: I9f50e9dd901280b5c32576e43165292299922995 Gerrit-Change-Number: 14212 Gerrit-PatchSet: 1 Gerrit-Owner: Volodymyr Verovkin
