Gabriella Lotz has posted comments on this change. ( http://gerrit.cloudera.org:8080/24246 )
Change subject: KUDU-3767 Add global leader awareness to auto leader rebalancer (Part 1) ...................................................................... Patch Set 6: (4 comments) http://gerrit.cloudera.org:8080/#/c/24246/4/src/kudu/master/auto_leader_rebalancer-test.cc File src/kudu/master/auto_leader_rebalancer-test.cc: http://gerrit.cloudera.org:8080/#/c/24246/4/src/kudu/master/auto_leader_rebalancer-test.cc@657 PS4, Line 657: for (const auto& uuid : tserver_uuids) { > q: This assertion (ts2 receives the leader) only fails without the patch if Good catch, it wasn't deterministic. The follower order comes from interned_replicas, which just follows the Raft config peer order, and since the tserver uuids are random there was no guarantee ts1 came before ts2. So the old test really was a coin flip on whether it caught a regression. I took the dependency out entirely. The destination selection is now order-independent: when two followers tie for the table, it breaks the tie on the overall leader count, and if that's still tied, on the uuid. So the result is the same no matter what order the replicas come back in. I also reworked the test around that. It now loads the smaller-uuid follower and checks that the larger-uuid one gets the leader. With the tie breaker, the larger-uuid tserver wins because it has fewer leaders overall; without it, the tie falls through to the uuid comparison and the smaller-uuid (already loaded) tserver wins, so the check fails. I confirmed this by temporarily removing the tie breaker, the test fails every run. One side note: this also turned up a bug in MakeLeaderDistribution, which couldn't express a distribution with an interior zero like {1,0,1}. Fixed that too. http://gerrit.cloudera.org:8080/#/c/24246/4/src/kudu/master/auto_leader_rebalancer.cc File src/kudu/master/auto_leader_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/24246/4/src/kudu/master/auto_leader_rebalancer.cc@214 PS4, Line 214: // Count this table's leaders into the global map before the early return for > q: The global_leader_count accumulation happens after the early-return at t Not intentional, it fell out of the old replication_factor == 1 early return. Fixed it, RF==1 leaders are now counted into the global map before the early return, so they're visible to the tie-breaker; the transfer is still skipped since they're immovable. http://gerrit.cloudera.org:8080/#/c/24246/4/src/kudu/master/auto_leader_rebalancer.cc@286 PS4, Line 286: for (int i = 0; i < uuid_leaders.size(); i++) { > nit: min_global_count is initialized to 0. If the first follower examined h Yes, intended. If every candidate follower is fully loaded (ratio 1.0), none is selected and the transfer is skipped, which is correct since moving a leader there would only worsen skew. http://gerrit.cloudera.org:8080/#/c/24246/4/src/kudu/master/auto_leader_rebalancer.cc@303 PS4, Line 303: // comparing rounded doubles would not. > q: The tie-breaker fires only on `score == min_score` (exact double equalit Switched to integer cross multiplication for both the ratio comparison and the skew check, so the "double is not precise" caveat is gone and ties are exact by construction. -- To view, visit http://gerrit.cloudera.org:8080/24246 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I47e266d2a06d5c18a50270e0d5e9d4954480f308 Gerrit-Change-Number: 24246 Gerrit-PatchSet: 6 Gerrit-Owner: Gabriella Lotz <[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-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Mon, 08 Jun 2026 09:17:38 +0000 Gerrit-HasComments: Yes
