[kudu-CR] KUDU-3729 Prefer follower moves in auto-rebalancing

["Alexey Serbin (Code Review)" <ge...@cloudera.org>]

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 <lo...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Comment-Date: Wed, 08 Apr 2026 01:26:35 +0000
Gerrit-HasComments: Yes