Marton Greber 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 1: (2 comments) What do you think about adding test coverage for the following scenarios: 1. Global pass skipped when per-table has work -> The gating condition (per_table_moves_scheduled == 0) is untested. A test that creates both per-table skew and global skew, runs one round, and asserts that only the per-table moves fire (global pass deferred) would lock down that contract. Without it, someone could accidentally remove the gate and both paths would race on the same tablets. 2. Maintenance mode during global pass -> The existing TestMaintenanceMode covers per-table only. There's no test that a maintenance-mode tserver is excluded as a global-pass destination (line 654 check) or that it's excluded from the eligible pool for computing ceil_avg (line 469). If that exclusion logic broke, the pass could target a draining tserver. 3. remaining_moves cap respected -> No test sets leader_rebalancing_max_moves_per_round to a small value (e.g. 1) with many tablets needing global rebalancing, then asserts only that many moves fire per round. This would catch an off-by-one or a path that bypasses the cap. http://gerrit.cloudera.org:8080/#/c/24476/1/src/kudu/master/auto_leader_rebalancer-test.cc File src/kudu/master/auto_leader_rebalancer-test.cc: http://gerrit.cloudera.org:8080/#/c/24476/1/src/kudu/master/auto_leader_rebalancer-test.cc@709 PS1, Line 709: TEST_F(LeaderRebalancerTest, GlobalLeaderBalanceAcrossTables) { The two new tests share ~80% identical code (the global_distribution lambda, retry loop, and final assertions). A small helper parameterized on kNumTables/table-name-prefix would halve the test code. What do you think? http://gerrit.cloudera.org:8080/#/c/24476/1/src/kudu/master/auto_leader_rebalancer.cc File src/kudu/master/auto_leader_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/24476/1/src/kudu/master/auto_leader_rebalancer.cc@638 PS1, Line 638: (*global_leader_count)[source_uuid]--; : (*global_leader_count)[dest_uuid]++; : table_leader_count_by_ts[source_uuid]--; : table_leader_count_by_ts[dest_uuid]++; The bookkeeping updates here (global_leader_count[source]--, [dest]++) happen during planning, but execution below (line 650+) can skip moves if the destination enters maintenance mid-round or LookupTSByUUID fails. So the map can diverge from what actually transferred. I assume that's fine since the next round recomputes from scratch — worth a one-liner confirming that intent? -- 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: 1 Gerrit-Owner: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Tue, 23 Jun 2026 13:00:10 +0000 Gerrit-HasComments: Yes
