Gabriella Lotz has posted comments on this change. ( http://gerrit.cloudera.org:8080/24476 )
Change subject: KUDU-3767 Add global leader awareness to auto leader rebalancer (Part 2) ...................................................................... Patch Set 4: (12 comments) Thank you for the thorough review! http://gerrit.cloudera.org:8080/#/c/24476/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24476/1//COMMIT_MSG@7 PS1, Line 7: Add global leader awareness to auto leader rebalancer (Part 2) > Thank you for the responses. I think this is just the rebalancing interval. auto_leader_rebalancing_interval_seconds defaults to 3600 (1 hour), and the loop sleeps for that whole interval before its first run, so at approx. 30 minutes it hasn't executed a single rebalancing round yet. That's probably why nothing has moved. Could you try setting --auto_leader_rebalancing_interval_seconds=60 and re-checking? If it's still stuck after a round has definitely executed, let me know and I'll dig in further. http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer-test.cc File src/kudu/master/auto_leader_rebalancer-test.cc: http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer-test.cc@287 PS3, Line 287: vecto > nit for here and elsewhere: remove 'std::' prefix since there is 'using std Done http://gerrit.cloudera.org:8080/#/c/24476/2/src/kudu/master/auto_leader_rebalancer.cc File src/kudu/master/auto_leader_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/24476/2/src/kudu/master/auto_leader_rebalancer.cc@454 PS2, Line 454: us Au > nit: add 'using std::unordered_map' and get rid of std:: prefix to match th Done http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer.cc File src/kudu/master/auto_leader_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer.cc@450 PS3, Line 450: leader_transfer_count); > How does this work if there is a bunch of non-replicated (i.e. RF=1) tables RF=1 leaders are intentionally counted as load (so the pass won't pile movable leaders onto a tserver that's already busy with them), but they're never moved, since a single-replica tablet has no follower to hand leadership to. I think clustering many of them on one tserver just makes it look loaded: the pass makes no move for it rather than misbehaving. I've added GlobalLeaderBalanceIgnoresSingleReplicaTables, which mixes RF=1 and RF=3 tables and checks the RF=1 leaders stay put while the RF=3 tables still get balanced. http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer.cc@454 PS3, Line 454: us Au > nit: add 'using std::unordered_map;' in the 'using' section and remove 'st Done http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer.cc@456 PS3, Line 456: const vector<string>& tserver_uuids, : const unordered_set<string>& exclude_dest_uuids, : > For what use case this 'if' is present? Wouldn't it be a programming mista Done http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer.cc@491 PS3, Line 491: // A leader is only ever handed to a tserver that already hosts a follower > The ceil_avg is computed based on 'num_eligible' that might include complet Well, it can't move endlessly, two things prevent that. One is that a leader is only ever handed to a tserver that already hosts a follower of that tablet, the other is that every move strictly decreases the source's count and requires the destination to be below the ceiling. So the distribution can't oscillate. I've added a comment explaining this. http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer.cc@519 PS3, Line 519: > Why to have these casts -- wouldn't implicit casting do its job as expected Done http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer.cc@591 PS3, Line 591: remaining_moves > I don't think this cast is needed. Done http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer.cc@627 PS3, Line 627: follower_global_count < dest_global_count || > Here and elsewhere: consider wrapping this into PREDICT_FALSE if targeting Done http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer.cc@700 PS3, Line 700: RN_NOT_OK(s, Substitu > What if there was an error -- should we log about that? Good catch, added an else branch that logs the failed transfer at WARNING. http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer.cc@708 PS3, Line 708: ti > This looks like a typo: moves_scheduled_this_round_for_test_ is incremente Not a typo, but it was pointlessly defensive: in TEST mode the method returns early as soon as it finds a move, so if it reaches that line nothing was scheduled and it's a += 0. Removed the guard so it's unconditional and added a comment. -- To view, visit http://gerrit.cloudera.org:8080/24476 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5d44761f6444ca1d1bf4fcee405d65d029e41980 Gerrit-Change-Number: 24476 Gerrit-PatchSet: 4 Gerrit-Owner: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Wed, 08 Jul 2026 13:45:46 +0000 Gerrit-HasComments: Yes
