Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/24613 )
Change subject: KUDU-3760 expose follower-move flag in rebalancer ...................................................................... Patch Set 2: (2 comments) Also a comment for a part which is not explicitly touched by this patch: File: src/kudu/tools/tool_action_cluster.cc Line: 455 Comment: The new --rebalancing_prefer_follower_replica_moves flag is not registered on the rebalance action via .AddOptionalParameter(). Every sibling rebalancer flag (load_imbalance_threshold, max_moves_per_server, disable_intra_location_rebalancing, move_single_replicas, ...) is listed here, which is what surfaces them in `kudu cluster rebalance --help` and in the generated flag/XML docs. As written the flag works (gflags parses it globally, which is why the test's s.ok() passes) but is invisible to --help and undocumented. Since the whole point of the commit is to make the option "explicit and overridable from the command line," shouldn't it be added here too? (AddOptionalParameter only needs the flag name at runtime, so no DECLARE is required even though it's DEFINE'd in rebalancer_tool.cc.) http://gerrit.cloudera.org:8080/#/c/24613/2/src/kudu/tools/rebalancer_tool-test.cc File src/kudu/tools/rebalancer_tool-test.cc: http://gerrit.cloudera.org:8080/#/c/24613/2/src/kudu/tools/rebalancer_tool-test.cc@795 PS2, Line 795: q: this test guards that the CLI accepts the flag (an unregistered flag would abort the tool) and that the cluster ends up balanced either way -- but as the comment notes, both flag values must produce a balanced cluster, so it would still pass even if the flag were plumbed to the wrong runner or ignored entirely. That's fine given rebalance_algo-test.cc covers the actual leader-vs-follower behavior; worth being aware the integration test's guard is weaker than it looks (it's really an "is-the-flag-wired-to-the-CLI" regression test, which matches the commit's stated intent). http://gerrit.cloudera.org:8080/#/c/24613/2/src/kudu/tools/rebalancer_tool.cc File src/kudu/tools/rebalancer_tool.cc: http://gerrit.cloudera.org:8080/#/c/24613/2/src/kudu/tools/rebalancer_tool.cc@1346 PS2, Line 1346: deadline nit: this changed `std::move(deadline)` to `deadline` (plain copy) while the sibling CrossLocationRunner/ReplaceBasedRunner ctors still `std::move(deadline)`. optional<MonoTime> is cheap to copy so it's harmless, but the change looks incidental and is now inconsistent with the neighbors -- was dropping the move intentional? -- To view, visit http://gerrit.cloudera.org:8080/24613 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5f1b03da71b11af82f18ff9e20dcaa676b185645 Gerrit-Change-Number: 24613 Gerrit-PatchSet: 2 Gerrit-Owner: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[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: Thu, 30 Jul 2026 13:54:16 +0000 Gerrit-HasComments: Yes
