Alexey Serbin 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 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/10266/4/src/kudu/cfile/block_cache.cc File src/kudu/cfile/block_cache.cc: http://gerrit.cloudera.org:8080/#/c/10266/4/src/kudu/cfile/block_cache.cc@67 PS4, Line 67: int64 int64_t ? http://gerrit.cloudera.org:8080/#/c/10266/4/src/kudu/cfile/block_cache.cc@72 PS4, Line 72: nit: extra space http://gerrit.cloudera.org:8080/#/c/10266/4/src/kudu/cfile/block_cache.cc@72 PS4, Line 72: : return false; Do you think it's worth to provide an ability to override this and run with those pernicious effects if a special --force_xxx was specified? http://gerrit.cloudera.org:8080/#/c/10266/4/src/kudu/cfile/block_cache.cc@77 PS4, Line 77: nit: extra space http://gerrit.cloudera.org:8080/#/c/10266/4/src/kudu/integration-tests/client-stress-test.cc File src/kudu/integration-tests/client-stress-test.cc: http://gerrit.cloudera.org:8080/#/c/10266/4/src/kudu/integration-tests/client-stress-test.cc@241 PS4, Line 241: // The block cache capacity needs to be disabled. See the group validator : // in cfile/block_cache.cc. : opts.extra_tserver_flags.push_back("--block_cache_capacity_mb=0"); Does it induce some crucial changes in the behavior of the system while running this test? http://gerrit.cloudera.org:8080/#/c/10266/4/src/kudu/integration-tests/raft_consensus-itest.cc File src/kudu/integration-tests/raft_consensus-itest.cc: http://gerrit.cloudera.org:8080/#/c/10266/4/src/kudu/integration-tests/raft_consensus-itest.cc@1810 PS4, Line 1810: "--block_cache_capacity_mb=0", Why not just move one "--block_cache_capacity_mb=0" out of the #ifdefs ? http://gerrit.cloudera.org:8080/#/c/10266/4/src/kudu/util/process_memory.cc File src/kudu/util/process_memory.cc: http://gerrit.cloudera.org:8080/#/c/10266/4/src/kudu/util/process_memory.cc@a182 PS4, Line 182: : : : : : : : Wasn't it useful at all? In other words, do we print that info from somewhere else? -- 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: 4 Gerrit-Owner: Will Berkeley <[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:20:36 +0000 Gerrit-HasComments: Yes
