Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10266 )
Change subject: Add validation and docs on setting block_cache_capacity_mb too high ...................................................................... Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/10266/5/src/kudu/cfile/block_cache.cc File src/kudu/cfile/block_cache.cc: http://gerrit.cloudera.org:8080/#/c/10266/5/src/kudu/cfile/block_cache.cc@41 PS5, Line 41: DEFINE_bool(force_block_cache_capacity_mb, false, Nit: the _mb suffix makes me think this is a megabyte quantity. http://gerrit.cloudera.org:8080/#/c/10266/5/src/kudu/cfile/block_cache.cc@77 PS5, Line 77: LOG(ERROR) << "Block cache capacity exceeds the memory pressure threshold " Nit: use strings::Substitute() here and below, makes the text a little more clear IMHO. http://gerrit.cloudera.org:8080/#/c/10266/5/src/kudu/cfile/block_cache.cc@94 PS5, Line 94: // Group validation is necessary since the validator depends on : // --memory_limit_hard_bytes and other memory flags. FWIW, I think this is implied by the content of the validator. http://gerrit.cloudera.org:8080/#/c/10266/5/src/kudu/integration-tests/client-stress-test.cc File src/kudu/integration-tests/client-stress-test.cc: http://gerrit.cloudera.org:8080/#/c/10266/5/src/kudu/integration-tests/client-stress-test.cc@241 PS5, Line 241: // The block cache capacity needs to be disabled. See the group validator Hmm, this just disables the block cache outright, correct? Reword to say that? Also, please add a few more words about _why_ it needs to be disabled. http://gerrit.cloudera.org:8080/#/c/10266/5/src/kudu/integration-tests/raft_consensus-itest.cc File src/kudu/integration-tests/raft_consensus-itest.cc: http://gerrit.cloudera.org:8080/#/c/10266/5/src/kudu/integration-tests/raft_consensus-itest.cc@1803 PS5, Line 1803: // We also need to limit the block cache capacity in tandem. See the group Again, isn't this outright disabling? -- To view, visit http://gerrit.cloudera.org:8080/10266 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia99fa95c578235beb3a1c72a33a22b4d8ff4800c Gerrit-Change-Number: 10266 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Thu, 03 May 2018 18:49:58 +0000 Gerrit-HasComments: Yes
