Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/24319 )
Change subject: KUDU-3730 Add observability counters to the auto-rebalancer ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/24319/1/src/kudu/master/auto_rebalancer-test.cc File src/kudu/master/auto_rebalancer-test.cc: http://gerrit.cloudera.org:8080/#/c/24319/1/src/kudu/master/auto_rebalancer-test.cc@1433 PS1, Line 1433: Can you please add a bit more test coverage? 1. Add a test where leader moves are actually chosen (e.g. RF=1 workload or a scenario with no follower candidates on source). Assert: auto_rebalancer_leader_moves_scheduled > 0, and follower counter stays at 0 (or at least leader increases from baseline). 2. Use the existing failure injection (auto_rebalancing_fail_moves_for_test=true) and verify no move counter increments when scheduling fails. Assert: leader/follower counters unchanged from baseline even if moves_attempted_this_round_for_test_ > 0. 3. Force a round to hit a continue path (e.g. cluster info / move retrieval failure condition already exercised in existing tests), then check auto_rebalancer_rounds_completed unchanged. This is important because the metric is intended as “full rounds completed”. 4. In a stable balanced cluster, verify rounds still increment over time while move counters do not. This confirms the metric semantics and avoids operator confusion (“is the rebalancer running at all?”). http://gerrit.cloudera.org:8080/#/c/24319/1/src/kudu/master/auto_rebalancer.cc File src/kudu/master/auto_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/24319/1/src/kudu/master/auto_rebalancer.cc@175 PS1, Line 175: kudu::MetricUnit::kTablets, here and below: I might not have the best understanding here but these counters increment per move operation, not per unique tablet. Is MetricUnit::kTablets still the intended unit, or should this be kUnits? http://gerrit.cloudera.org:8080/#/c/24319/1/src/kudu/rebalance/rebalancer.h File src/kudu/rebalance/rebalancer.h: http://gerrit.cloudera.org:8080/#/c/24319/1/src/kudu/rebalance/rebalancer.h@193 PS1, Line 193: bool FindReplicas(const TableReplicaMove& move, If I saw correctly rebalancer_tool.cc now calls FindReplicas() and ignores the new bool return. That can be intentional, but it should be made explicit with a short comment why. http://gerrit.cloudera.org:8080/#/c/24319/1/src/kudu/rebalance/rebalancer.h@305 PS1, Line 305: false That default can silently mislabel callers that forget to pass it. Requiring explicit passing (no default) would be safer, or at least documenting why default false is acceptable for tool callers. -- To view, visit http://gerrit.cloudera.org:8080/24319 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I34ab527033ed71b1b591d86387f7f3af2dd4f6a3 Gerrit-Change-Number: 24319 Gerrit-PatchSet: 1 Gerrit-Owner: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Wed, 20 May 2026 15:35:40 +0000 Gerrit-HasComments: Yes
