Song Jiacheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/20310 )
Change subject: KUDU-3497 optimize leader rebalancer algorithm ...................................................................... Patch Set 11: (9 comments) http://gerrit.cloudera.org:8080/#/c/20310/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20310/10//COMMIT_MSG@10 PS10, Line 10: : For instance, for > You mean RF=1, i.e. non-replication tables here? It would be great to clar Thank you so much for the review! And done. http://gerrit.cloudera.org:8080/#/c/20310/10//COMMIT_MSG@12 PS10, Line 12: > consisting Done http://gerrit.cloudera.org:8080/#/c/20310/10//COMMIT_MSG@12 PS10, Line 12: : Its le > nit: replication factor of 3 Done http://gerrit.cloudera.org:8080/#/c/20310/10/src/kudu/master/auto_leader_rebalancer.cc File src/kudu/master/auto_leader_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/20310/10/src/kudu/master/auto_leader_rebalancer.cc@20 PS10, Line 20: #include <algorithm> : #include <cmath> > nit: move these two lines between the <algorithm> and the <cstdint> include Done http://gerrit.cloudera.org:8080/#/c/20310/10/src/kudu/master/auto_leader_rebalancer.cc@233 PS10, Line 233: r th > per Done http://gerrit.cloudera.org:8080/#/c/20310/10/src/kudu/master/auto_leader_rebalancer.cc@233 PS10, Line 233: Transf > leader replicas Done http://gerrit.cloudera.org:8080/#/c/20310/10/src/kudu/master/auto_leader_rebalancer.cc@236 PS10, Line 236: int32_t should_transfer_count = leader_count - target_leader_count; : if (should_transfer_count > 0) { : leader_transfer_source.insert({uuid, should_transfer_count}); : VLOG(1) << Substitute("$0 should transfer leader count: $1", uuid, should_transfer_count); : } : } : > Could this be replaced with Done. http://gerrit.cloudera.org:8080/#/c/20310/10/src/kudu/master/auto_leader_rebalancer.cc@243 PS10, Line 243: > nit: consider renaming to 'target_leader_count' Done http://gerrit.cloudera.org:8080/#/c/20310/10/src/kudu/master/auto_leader_rebalancer.cc@248 PS10, Line 248: int32_t need_transfer_count = from_info.second; : int32_t pick_count = 0; : vector<string>& uuid_leaders = leader_tablet_ids_by_ts_uuid[leader_uuid]; : std::shuffle(uuid_leaders.begin(), uuid_leaders.end(), random_generator_); : // This loop would generate 'uuid_leaders.size()' leader transferring tasks at most. : // Every task would pick a dest uuid to transfer leader. : for (int i = 0; i < u > I'm not sure this 'thinning' approach makes a lot of sense semantically. A Constant values are better indeed, done. -- To view, visit http://gerrit.cloudera.org:8080/20310 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0f1fe796fd98da2d8764da793b7e254319e6348a Gerrit-Change-Number: 20310 Gerrit-PatchSet: 11 Gerrit-Owner: Song Jiacheng <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Tue, 17 Oct 2023 07:26:48 +0000 Gerrit-HasComments: Yes
