Marton Greber 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 5: Code-Review+1 (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: ASSERT_EQ(1, dist2.at(ts2_uuid)); q: This assertion (ts2 receives the leader) only fails without the patch if the follower iteration order for table2's tablets puts ts1 before ts2 - otherwise iteration order would route to ts2 anyway and the test passes even with the tie-breaker removed. Did you confirm the order is deterministic here (it derives from GetTabletLocations / interned_replicas ordering)? 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: if (global_leader_count) { q: The global_leader_count accumulation happens after the early-return at the top of this function (RF==1 or REMOVED tables return before this point). That means leaders of RF==1 tables never contribute to the global view, so a tserver hosting many single-replica leaders looks "empty" to the tie-breaker and will be preferentially picked as a transfer destination. Is excluding RF==1 leaders intentional? They're immovable but they still represent real leader load we presumably want to balance around. http://gerrit.cloudera.org:8080/#/c/24246/4/src/kudu/master/auto_leader_rebalancer.cc@286 PS4, Line 286: int min_global_count = 0; nit: min_global_count is initialized to 0. If the first follower examined has min_score's initial value (score == 1.0, i.e. fully loaded) its `global_count < 0` check can never be true, so it's never selected — same as the pre-patch behavior, just worth confirming this edge (all followers at score 1.0) is intended to skip the transfer. http://gerrit.cloudera.org:8080/#/c/24246/4/src/kudu/master/auto_leader_rebalancer.cc@303 PS4, Line 303: if (score < min_score || (score == min_score && global_count < min_global_count)) { q: The tie-breaker fires only on `score == min_score` (exact double equality). Two candidates with mathematically equal ratios but different denominators (e.g. 1/3 vs 2/6) may differ by an ULP and silently skip the tie-break, so the global awareness wouldn't kick in for those cases. Given the existing "double is not precise" comment, would comparing with a small epsilon (or comparing leader_count*other_replica vs other_leader*replica as integers) make the tie detection robust? -- 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: 5 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, 01 Jun 2026 12:36:37 +0000 Gerrit-HasComments: Yes
