Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14154 )
Change subject: KUDU-2914: Rebalance tool support moving replicas from some specific tablet servers ...................................................................... Patch Set 10: (10 comments) http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/rebalance/rebalance-test.cc File src/kudu/rebalance/rebalance-test.cc: http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/rebalance/rebalance-test.cc@192 PS10, Line 192: bool HasSameContents(const multimap<int32_t, TableBalanceInfo>& lhs, I think it's worth documenting the essence of what this function uses as the lhs-to-rhs comparision criteria. http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool-test.cc File src/kudu/tools/rebalancer_tool-test.cc: http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool-test.cc@305 PS10, Line 305: bool is_343_scheme = true; : const vector<string> kMasterFlags = { : Substitute("--raft_prepare_replacement_before_eviction=$0", is_343_scheme), : }; : const vector<string> kTserverFlags = { : Substitute("--raft_prepare_replacement_before_eviction=$0", is_343_scheme), : }; This is not necessary: newer Kudu clusters runs in 343 mode by default. http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool-test.cc@355 PS10, Line 355: "--ignored_tservers=" + : cluster_->tablet_server(0)->uuid(), nit: join there two lines for better readability http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool-test.cc@360 PS10, Line 360: ASSERT_STR_CONTAINS(out, Substitute("$0 | 0", cluster_->tablet_server(0)->uuid())) : << "stderr: " << err; Add a comment explaining that this line show that the number of tablet replicas at the ignored tserver is 0 after the rebalancing. http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool-test.cc@372 PS10, Line 372: ts->Shutdown(); BTW, what happens if a tablet server, which is ignored and healthy in the beginning, goes down during the rebalancing process? Maybe, add a test scenario to be explicit of what behavior is expected in such case. http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool.cc File src/kudu/tools/rebalancer_tool.cc: http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool.cc@188 PS10, Line 188: on nit: from http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool.cc@1294 PS10, Line 1294: RETURN_NOT_OK(GetReplaceMoves(ci, raw_info, replica_moves)); : return Status::OK(); nit: shorten this to return GetReplaceMoves(ci, raw_info, replica_moves); http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool.cc@1420 PS10, Line 1420: helathy healthy http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool.cc@1421 PS10, Line 1421: // maintenance(or decommision) mode. nit: add a space before the parenthesis http://gerrit.cloudera.org:8080/#/c/14154/10/src/kudu/tools/rebalancer_tool.cc@1506 PS10, Line 1506: ReplicaMove move = {tablets_info[i].tablet_id, elem.first, "", tablets_info[i].config_idx}; : result_moves.emplace_back(std::move(move)); nit: would the following work here result_moves.emplace_back(tablets_info[i].tablet_id, elem.first, "", tablets_info[i].config_idx); ? -- To view, visit http://gerrit.cloudera.org:8080/14154 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I86cfb740030946c13db1a9ca63d241f4907d6c89 Gerrit-Change-Number: 14154 Gerrit-PatchSet: 10 Gerrit-Owner: Yifan Zhang <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hannah Nguyen <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Comment-Date: Fri, 25 Oct 2019 01:27:26 +0000 Gerrit-HasComments: Yes
