Yuqi Du 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: (13 comments) Thank you for you advices. I have fixed them, You can review them again. 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 Done http://gerrit.cloudera.org:8080/#/c/18454/35/src/kudu/master/auto_leader_rebalancer.h@45 PS35, Line 45: . > nit: remove Done http://gerrit.cloudera.org:8080/#/c/18454/35/src/kudu/master/auto_leader_rebalancer.h@95 PS35, Line 95: ’LeaderStepDown‘ > nit: 'LeaderStepDown' Done 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 "{}"? 'auto& leader_uuids' is a left reference , it cannot be '{}' 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_uu Done 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? Done 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? Done 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 ? Done http://gerrit.cloudera.org:8080/#/c/18454/35/src/kudu/master/auto_leader_rebalancer.cc@256 PS35, Line 256: pair > same Done 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 ?? 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 ? At this, I think it's no distinguish. ` auto it = host_port_by_leader_ts_uuid.find(leader_uuid); if (it == host_port_by_leader_ts_uuid.end()) { continue; } or auto* host_port = FindOrNull(host_port_by_leader_ts_uuid, leader_uuid); if (!host_port) { continue; } ` Fixed. 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 examp 'RunLeaderRebalanceForTable' has printed the detail log. Yes, Next patch can try to rerich the log and add metrics for leader rebalancer. 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 an The only failed status is 'RETURN_NOT_OK(leader_lock.first_failed_status());' so I direct change the log. -- 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: Thu, 10 Nov 2022 11:01:41 +0000 Gerrit-HasComments: Yes
