Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18454 )
Change subject: [master] KUDU-3390 support auto rebalance tablet leaders across TServers ...................................................................... Patch Set 35: (12 comments) http://gerrit.cloudera.org:8080/#/c/18454/35/src/kudu/master/auto_leader_rebalancer.h File src/kudu/master/auto_leader_rebalancer.h: http://gerrit.cloudera.org:8080/#/c/18454/35/src/kudu/master/auto_leader_rebalancer.h@45 PS35, Line 45: . nit: remove http://gerrit.cloudera.org:8080/#/c/18454/35/src/kudu/master/auto_leader_rebalancer.h@95 PS35, Line 95: ’LeaderStepDown‘ nit: 'LeaderStepDown' http://gerrit.cloudera.org:8080/#/c/18454/35/src/kudu/master/auto_leader_rebalancer.cc File src/kudu/master/auto_leader_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/18454/35/src/kudu/master/auto_leader_rebalancer.cc@167 PS35, Line 167: vector<string>() nit: can be simplified by "{}"? http://gerrit.cloudera.org:8080/#/c/18454/35/src/kudu/master/auto_leader_rebalancer.cc@191 PS35, Line 191: statistics 'statistics' is not very clear, how about replica_and_leader_count_by_ts_uuid or sth like this ? http://gerrit.cloudera.org:8080/#/c/18454/35/src/kudu/master/auto_leader_rebalancer.cc@223 PS35, Line 223: expected_count nit: how about need_transfer_count? http://gerrit.cloudera.org:8080/#/c/18454/35/src/kudu/master/auto_leader_rebalancer.cc@227 PS35, Line 227: pick count leader, and pick dest uuid The comment is not clear enough, could you improve it? http://gerrit.cloudera.org:8080/#/c/18454/35/src/kudu/master/auto_leader_rebalancer.cc@239 PS35, Line 239: pair nit: replica_and_leader_count ? http://gerrit.cloudera.org:8080/#/c/18454/35/src/kudu/master/auto_leader_rebalancer.cc@256 PS35, Line 256: pair same http://gerrit.cloudera.org:8080/#/c/18454/35/src/kudu/master/auto_leader_rebalancer.cc@295 PS35, Line 295: if (leader_transfer_count >= FLAGS_leader_rebalancing_max_moves_per_round) { How about limit the leader_transfer_tasks size when collect it? then reduce the cost of collection as well. http://gerrit.cloudera.org:8080/#/c/18454/35/src/kudu/master/auto_leader_rebalancer.cc@312 PS35, Line 312: auto it = host_port_by_leader_ts_uuid.find(leader_uuid); Use FindOrNull ? http://gerrit.cloudera.org:8080/#/c/18454/35/src/kudu/master/auto_leader_rebalancer.cc@381 PS35, Line 381: LOG(INFO) << "all tables leader rebalance finished this round"; It would be better if printing the summary of the rebalance work, for example, how many replicas switch leadership. You can do it in another patch. http://gerrit.cloudera.org:8080/#/c/18454/35/src/kudu/master/auto_leader_rebalancer.cc@390 PS35, Line 390: Something wrong, maybe the master instance isn't leader Will RunLeaderRebalancer return the corresponding error? use it directly and not guess what the reason is, it will make user confused if they are not match. -- To view, visit http://gerrit.cloudera.org:8080/18454 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibfb60d8759a93b6a19238637c27df4f6b1cac918 Gerrit-Change-Number: 18454 Gerrit-PatchSet: 35 Gerrit-Owner: Yuqi Du <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Tue, 08 Nov 2022 08:33:36 +0000 Gerrit-HasComments: Yes
