Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18441 )
Change subject: [tserver] KUDU-1827: tserver decommission ...................................................................... Patch Set 12: (11 comments) http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/master/auto_rebalancer.cc File src/kudu/master/auto_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/master/auto_rebalancer.cc@478 PS12, Line 478: ts_manager_->GetDecommissionedCount() Does it mean it's OK to put replicas on tablet servers being de-commissioned? http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/master/master.proto@1042 PS12, Line 1042: At any time only one TS should be in this state to avoid resource : // contention and unnecessary moves when decommissioning multiple instances. Is this invariant enforced in the code? http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/master/ts_manager.cc File src/kudu/master/ts_manager.cc: http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/master/ts_manager.cc@227 PS12, Line 227: for (auto const& entry : stateMap) { : // TServerStateMap entry schema is: <uuid, <state, timestamp>> You could use structure binding for this instead: Kudu code is using C++17 internally. http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/rebalance/rebalancer.h File src/kudu/rebalance/rebalancer.h: http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/rebalance/rebalancer.h@219 PS12, Line 219: std::string This could return 'const std::string&' to avoid useless copying. http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/rebalance/rebalancer.cc File src/kudu/rebalance/rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/rebalance/rebalancer.cc@539 PS12, Line 539: std::string Rebalancer::RunStatusAsString(Rebalancer::RunStatus run_status) { : static const char *enum_str[] = : { "Unknown", "Cluster is balanced", "Timed out" }; The standard way of doing this is using switch/case statement -- it eliminates many issues that another approach you use introduces. http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/rebalance/rebalancer.cc@543 PS12, Line 543: string tmp nit: there is no need for this cast from char* to string http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/rebalance/rebalancer.cc@543 PS12, Line 543: run_status What if this is out of the range of the enum_str array? http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/tools/rebalancer_tool.cc File src/kudu/tools/rebalancer_tool.cc: http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/tools/rebalancer_tool.cc@1625 PS12, Line 1625: maintenance mode How does this relate to the decommissioning? Is this message still actionable with the recent updates? http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/tools/tool_action_common.cc File src/kudu/tools/tool_action_common.cc: http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/tools/tool_action_common.cc@316 PS12, Line 316: const char* const kTServerAddressesArg = "tserver_addresses"; : const char* const kTServerAddressesDesc = "Comma-separated list of Kudu Tablet Servers where each " : "address is of form 'hostname:port'."; This seems to be a duplicate of what's currently in tool_action_master.cc -- the only difference is the separator. Maybe, consolidate that by just moving the corresponding definitions from tool_action_master.cc into this file instead? http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/tools/tool_action_tserver.cc File src/kudu/tools/tool_action_tserver.cc: http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/tools/tool_action_tserver.cc@19 PS12, Line 19: nit: remove the extra empty line http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/tools/tool_action_tserver.cc@74 PS12, Line 74: UUIDs of tablet servers In what form and what's the separator? -- To view, visit http://gerrit.cloudera.org:8080/18441 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I15c52b653c20b2a3a45fbf8934f19f6bd1a9caea Gerrit-Change-Number: 18441 Gerrit-PatchSet: 12 Gerrit-Owner: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Tue, 25 Jul 2023 22:10:11 +0000 Gerrit-HasComments: Yes
