Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11975 )
Change subject: [tools] Add tool to dump memtrackers ...................................................................... Patch Set 1: (12 comments) http://gerrit.cloudera.org:8080/#/c/11975/1/src/kudu/server/generic_service.cc File src/kudu/server/generic_service.cc: http://gerrit.cloudera.org:8080/#/c/11975/1/src/kudu/server/generic_service.cc@259 PS1, Line 259: const auto root = server_->mem_tracker(); const auto* http://gerrit.cloudera.org:8080/#/c/11975/1/src/kudu/server/generic_service.cc@260 PS1, Line 260: if (root) { Isn't this something we can assume? http://gerrit.cloudera.org:8080/#/c/11975/1/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/11975/1/src/kudu/tools/kudu-admin-test.cc@2060 PS1, Line 2060: TEST_F(AdminCliTest, TestDumpMemTrackers) { Should also update kudu-tool-test's "help" tests. http://gerrit.cloudera.org:8080/#/c/11975/1/src/kudu/tools/tool_action_diagnose.cc File src/kudu/tools/tool_action_diagnose.cc: http://gerrit.cloudera.org:8080/#/c/11975/1/src/kudu/tools/tool_action_diagnose.cc@47 PS1, Line 47: DECLARE_int64(timeout_ms); Shouldn't this be exposed via AddOptionalParameter() too? http://gerrit.cloudera.org:8080/#/c/11975/1/src/kudu/tools/tool_action_diagnose.cc@49 PS1, Line 49: DEFINE_string(output, "table", It's a little funky to describe the format option here. Why not override the description for format using an AddOptionalParameter override? http://gerrit.cloudera.org:8080/#/c/11975/1/src/kudu/tools/tool_action_diagnose.cc@155 PS1, Line 155: If the port is " : "omitted, the default tablet server port is used" So historically we've duplicated these kinds of tools into 'master' and 'tserver' in order to get the right default port for each. The actual tool body goes into tool_action_common.cc so the code isn't actually duplicated. http://gerrit.cloudera.org:8080/#/c/11975/1/src/kudu/util/mem_tracker-test.cc File src/kudu/util/mem_tracker-test.cc: http://gerrit.cloudera.org:8080/#/c/11975/1/src/kudu/util/mem_tracker-test.cc@129 PS1, Line 129: Maybe also tickle the consumption/peak_consumption values a bit? http://gerrit.cloudera.org:8080/#/c/11975/1/src/kudu/util/mem_tracker.cc File src/kudu/util/mem_tracker.cc: http://gerrit.cloudera.org:8080/#/c/11975/1/src/kudu/util/mem_tracker.cc@201 PS1, Line 201: auto& tracker_pb = tracker_and_pb.second; Not auto* ? http://gerrit.cloudera.org:8080/#/c/11975/1/src/kudu/util/mem_tracker.proto File src/kudu/util/mem_tracker.proto: http://gerrit.cloudera.org:8080/#/c/11975/1/src/kudu/util/mem_tracker.proto@19 PS1, Line 19: package kudu; Should probably also set a java_package. http://gerrit.cloudera.org:8080/#/c/11975/1/src/kudu/util/mem_tracker.proto@22 PS1, Line 22: // Note that recursive message are not a good idea in general because messages http://gerrit.cloudera.org:8080/#/c/11975/1/src/kudu/util/mem_tracker.proto@24 PS1, Line 24: a maximum : // depth of 3 This isn't enforced by memtrackers so it could conceivably change in the future. http://gerrit.cloudera.org:8080/#/c/11975/1/src/kudu/util/mem_tracker.proto@29 PS1, Line 29: required string id = 1; I think our guidance is to avoid introducing new 'required' fields, even if they're required by the application. -- To view, visit http://gerrit.cloudera.org:8080/11975 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3e54f809bb6434b8d8e8c95771fe089c9da122d0 Gerrit-Change-Number: 11975 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mitch Barnett <[email protected]> Gerrit-Comment-Date: Wed, 21 Nov 2018 16:48:28 +0000 Gerrit-HasComments: Yes
