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 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/14154/2/src/kudu/rebalance/rebalancer.h File src/kudu/rebalance/rebalancer.h: http://gerrit.cloudera.org:8080/#/c/14154/2/src/kudu/rebalance/rebalancer.h@205 PS2, Line 205: is_healthy_move nit: document whether it's possible to specify 'nullptr' as an argument when calling this function. http://gerrit.cloudera.org:8080/#/c/14154/2/src/kudu/rebalance/rebalancer.cc File src/kudu/rebalance/rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/14154/2/src/kudu/rebalance/rebalancer.cc@157 PS2, Line 157: *is_healthy_move = false; This seems to be the only place where the output parameter is assigned. Does it make to assign the 'is_healthy_move' parameter in other cases as well? http://gerrit.cloudera.org:8080/#/c/14154/2/src/kudu/tools/rebalancer_tool.cc File src/kudu/tools/rebalancer_tool.cc: http://gerrit.cloudera.org:8080/#/c/14154/2/src/kudu/tools/rebalancer_tool.cc@951 PS2, Line 951: if (!cluster_info.healthy_ignored_tservers.empty()) { : // Check if it is safe to remove ignored tservers from the cluster. : int remaining_tservers_count = cluster_info.balance.servers_by_total_replica_count.size(); : if (remaining_tservers_count < max_replication_factor) { : return Status::InvalidArgument(Substitute( : "Too many ignored tservers.\nThe number of remaining tservers must be at least $0.", : max_replication_factor)); : } : RETURN_NOT_OK(algorithm()->MoveReplicasFromTservers(&cluster_info, : max_moves_per_server_ * 5, : &moves)); : } To simplify the matters, is it possible to move the replicas from the healthy ignored tservers before starting the rebalancing? Basically, that would be an extra step in the beginning. The optimal movements of those replicas might be output by some new specialized algo not necessarily the existing one. The new algo could have the same interface, but different implementation for GetNextMoves() method. I think doing it that way would provide separation of responsibilities and keep the code simpler. What do you think? -- 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: 2 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: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Comment-Date: Fri, 06 Sep 2019 01:00:20 +0000 Gerrit-HasComments: Yes
