Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20310 )
Change subject: KUDU-3497 optimize leader rebalancer algorithm ...................................................................... Patch Set 2: (16 comments) http://gerrit.cloudera.org:8080/#/c/20310/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20310/2//COMMIT_MSG@9 PS2, Line 9: especilally especially http://gerrit.cloudera.org:8080/#/c/20310/2//COMMIT_MSG@9 PS2, Line 9: rebalancer rebalancing http://gerrit.cloudera.org:8080/#/c/20310/2//COMMIT_MSG@10 PS2, Line 10: the tables who have less hash buckets and replication factors. the tables having smaller number of hash partitions and replication factor. http://gerrit.cloudera.org:8080/#/c/20310/2//COMMIT_MSG@33 PS2, Line 33: So the leader skew will never be more than 1. > Please add some unit tests to prove your fix is correct. +1 It would be nice to add a test -- that's not only to make sure the updated code addresses the issue described in the commit message, but also to catch possible regressions if the related code changes in the future. http://gerrit.cloudera.org:8080/#/c/20310/2/src/kudu/master/auto_leader_rebalancer.cc File src/kudu/master/auto_leader_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/20310/2/src/kudu/master/auto_leader_rebalancer.cc@204 PS2, Line 204: remain nit for this and the other's variable name: * change remain --> remaining * if using 'num' suffix for one variable, use it for the other as well if both semantically represent the number/count of some entities http://gerrit.cloudera.org:8080/#/c/20310/2/src/kudu/master/auto_leader_rebalancer.cc@204 PS2, Line 204: uint32_t nit: why not to use 'auto' or 'size_t' that's the return type of the size() method for std containers? http://gerrit.cloudera.org:8080/#/c/20310/2/src/kudu/master/auto_leader_rebalancer.cc@206 PS2, Line 206: who that http://gerrit.cloudera.org:8080/#/c/20310/2/src/kudu/master/auto_leader_rebalancer.cc@206 PS2, Line 206: rebalance rebalancing http://gerrit.cloudera.org:8080/#/c/20310/2/src/kudu/master/auto_leader_rebalancer.cc@206 PS2, Line 206: num number http://gerrit.cloudera.org:8080/#/c/20310/2/src/kudu/master/auto_leader_rebalancer.cc@209 PS2, Line 209: uint32_t replica_count = tablet_ids_ptr ? tablet_ids_ptr->size() : 0; nit: is it possible to use this criterion as-is in the condition for the 'if ()' below without introducing a variable? http://gerrit.cloudera.org:8080/#/c/20310/2/src/kudu/master/auto_leader_rebalancer.cc@228 PS2, Line 228: the remaining the number of remaining http://gerrit.cloudera.org:8080/#/c/20310/2/src/kudu/master/auto_leader_rebalancer.cc@228 PS2, Line 228: could be divided is divisible http://gerrit.cloudera.org:8080/#/c/20310/2/src/kudu/master/auto_leader_rebalancer.cc@228 PS2, Line 228: tablets tablet replicas http://gerrit.cloudera.org:8080/#/c/20310/2/src/kudu/master/auto_leader_rebalancer.cc@228 PS2, Line 228: the remaining the number of remaining http://gerrit.cloudera.org:8080/#/c/20310/2/src/kudu/master/auto_leader_rebalancer.cc@229 PS2, Line 229: leader num of all the remaining tablet servers number of leader http://gerrit.cloudera.org:8080/#/c/20310/2/src/kudu/master/auto_leader_rebalancer.cc@408 PS2, Line 408: this loop Imagine you are a person without the context of this code and you see this info message in the log. Would 'this loop' be confusing to you? Maybe, describe what 'this loop' actually is if you want to convey some useful information here to the users. Otherwise, maybe don't log anything here? If you need just a message for debugging purposes, consider using VLOG(...) here instead. -- 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: 2 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: Wang Xixu <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Tue, 08 Aug 2023 00:07:52 +0000 Gerrit-HasComments: Yes
