Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24085 )
Change subject: KUDU-3729 Prefer follower moves in auto-rebalancing ...................................................................... Patch Set 4: Code-Review+1 (11 comments) http://gerrit.cloudera.org:8080/#/c/24085/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24085/4//COMMIT_MSG@7 PS4, Line 7: Prefer follower moves in auto-rebalancing It's outside of this changelist's scope, but it seems to be a good place to ask: are you planning to add similar improvement into the kudu CLI tool as well? http://gerrit.cloudera.org:8080/#/c/24085/4/src/kudu/master/auto_rebalancer-test.cc File src/kudu/master/auto_rebalancer-test.cc: http://gerrit.cloudera.org:8080/#/c/24085/4/src/kudu/master/auto_rebalancer-test.cc@1290 PS4, Line 1290: const bool original_prefer = FLAGS_auto_rebalancing_prefer_follower_replica_moves; : SCOPED_CLEANUP({ FLAGS_auto_rebalancing_prefer_follower_replica_moves = original_prefer; }); Could the standard FlagSaver-based approach help here instead of this custom solution? http://gerrit.cloudera.org:8080/#/c/24085/4/src/kudu/master/auto_rebalancer-test.cc@1293 PS4, Line 1293: const int kNumOrigTservers = 3; : const int kNumTablets = 6; nit for here and elsewhere: make these constexpr http://gerrit.cloudera.org:8080/#/c/24085/4/src/kudu/master/auto_rebalancer-test.cc@1316 PS4, Line 1316: TEST_F(AutoRebalancerTest, TestReplicaRebalancingSchedulesMovesWithPreferFollowerFalse) { This and TestReplicaRebalancingSchedulesMovesWithPreferFollowerFalse scenario above have a lot of in common. Maybe, using a parameterized test could help to reduce the boiler-plate code? http://gerrit.cloudera.org:8080/#/c/24085/4/src/kudu/master/auto_rebalancer-test.cc@1317 PS4, Line 1317: const bool original_prefer = FLAGS_auto_rebalancing_prefer_follower_replica_moves; : SCOPED_CLEANUP({ FLAGS_auto_rebalancing_prefer_follower_replica_moves = original_prefer; }); ditto w.r.t. the google::FlagSaver approach http://gerrit.cloudera.org:8080/#/c/24085/4/src/kudu/master/auto_rebalancer-test.cc@1340 PS4, Line 1340: } // namespace master Does it make sense to add a scenario to verify how the updated algorithm works in case of non-replicated (i.e. RF=1) tables? That's at least is to make sure it doesn't show any unexpected behavior such as crash, etc. :) I'd think of purely RF=1 tables and also a hybrid/mixed case (say, one RF=3 table and several RF=1 tables). http://gerrit.cloudera.org:8080/#/c/24085/4/src/kudu/rebalance/rebalance_algo-test.cc File src/kudu/rebalance/rebalance_algo-test.cc: http://gerrit.cloudera.org:8080/#/c/24085/4/src/kudu/rebalance/rebalance_algo-test.cc@344 PS4, Line 344: {} nit: to keep TestClusterConfig definitions uniform across the file, consider using kNoLocations constant here as well We can switch to using {} everywhere (or updating the definition of kNoLocations) in a separate patch. http://gerrit.cloudera.org:8080/#/c/24085/4/src/kudu/rebalance/rebalance_algo-test.cc@364 PS4, Line 364: EXPECT_EQ nit: is there any particular idea behind using EXPECT_EQ() vs ASSERT_EQ() here? If not, consider switching to ASSERT_EQ() here as well. http://gerrit.cloudera.org:8080/#/c/24085/4/src/kudu/rebalance/rebalance_algo-test.cc@374 PS4, Line 374: EXPECT_EQ ditto http://gerrit.cloudera.org:8080/#/c/24085/4/src/kudu/rebalance/rebalance_algo-test.cc@383 PS4, Line 383: {} nit: use kNoLocations http://gerrit.cloudera.org:8080/#/c/24085/4/src/kudu/rebalance/rebalance_algo-test.cc@401 PS4, Line 401: EXPECT_EQ nit: since this is the last assertion in the scenario anyway, consider switching to ASSERT_EQ() here. -- To view, visit http://gerrit.cloudera.org:8080/24085 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0b56ae5ef8db5fe15ab25df1d1cebe84fd3b8f2c Gerrit-Change-Number: 24085 Gerrit-PatchSet: 4 Gerrit-Owner: Gabriella Lotz <[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-Comment-Date: Wed, 08 Apr 2026 01:26:35 +0000 Gerrit-HasComments: Yes
