Will Berkeley 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 ? Done http://gerrit.cloudera.org:8080/#/c/10266/4/src/kudu/cfile/block_cache.cc@72 PS4, Line 72: > nit: extra space Done 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 Sure, as an unsafe flag, if you wanted to see the effects or were working on a patch to mitigate them. Done. http://gerrit.cloudera.org:8080/#/c/10266/4/src/kudu/cfile/block_cache.cc@77 PS4, Line 77: > nit: extra space Done 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 run Don't think so. Since the soft limit percentage is 0, the servers are always under memory pressure anyway. 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 ? Done 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? It wasn't printed before this change AFAICT, and now the validator makes it get printed before logging is initialized, which is annoying. The info is available from flag values (and arithmetic). -- 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:31:14 +0000 Gerrit-HasComments: Yes
