Alexey Serbin 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 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer-test.cc File src/kudu/master/auto_leader_rebalancer-test.cc: http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer-test.cc@287 PS3, Line 287: std:: nit for here and elsewhere: remove 'std::' prefix since there is 'using std::vector' directive already http://gerrit.cloudera.org:8080/#/c/24476/2/src/kudu/master/auto_leader_rebalancer.cc File src/kudu/master/auto_leader_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/24476/2/src/kudu/master/auto_leader_rebalancer.cc@454 PS2, Line 454: std:: nit: add 'using std::unordered_map' and get rid of std:: prefix to match the rest of the parameters http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer.cc File src/kudu/master/auto_leader_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer.cc@450 PS3, Line 450: Status AutoLeaderRebalancerTask::RunGlobalLeaderRebalance( How does this work if there is a bunch of non-replicated (i.e. RF=1) tables in the cluster? Could non-replicated leader tablets introduce an unexpected behavior of this code (e.g., if almost all of them are clustered at the same tablet server, etc.)? It seems such tables' leaders are counted in, but no moves are performed on them at all due to the condition at line 530. Consider adding a few edge-case test scenarios involving non-replicated (RF=1) tables. http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer.cc@454 PS3, Line 454: std:: nit: add 'using std::unordered_map;' in the 'using' section and remove 'std::' suffix here and elsewhere in this file? http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer.cc@456 PS3, Line 456: if (!global_leader_count) { : return Status::OK(); : } For what use case this 'if' is present? Wouldn't it be a programming mistake to call this method with global_leader_count == nullptr? If it would be a mistake, DCHECK is a better way handling such situations, otherwise please add a comment to explain when it's needed. http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer.cc@491 PS3, Line 491: const int64_t ceil_avg = (total_leaders + num_eligible - 1) / num_eligible; The ceil_avg is computed based on 'num_eligible' that might include completely empty tablet servers or tablet servers that doesn't host any tablet replicas of relevant tablets. How to make sure that the code below doesn't try to endlessly move leadership from one node to another when there is a distribution of leader/follower counters like below, with non-replicated tables (RF=1) present: TS0 TS1 TS2 TS3 leadership count: 4 4 4 0 follower count: 2 2 2 0 empty: no no no yes http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer.cc@519 PS3, Line 519: static_cast<int> Why to have these casts -- wouldn't implicit casting do its job as expected here and elsewhere? http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer.cc@591 PS3, Line 591: static_cast<int> I don't think this cast is needed. http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer.cc@627 PS3, Line 627: mode == AutoLeaderRebalancerTask::ExecuteMode::TEST Here and elsewhere: consider wrapping this into PREDICT_FALSE if targeting the real-world workloads vs tests. http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer.cc@700 PS3, Line 700: !response.has_error() What if there was an error -- should we log about that? http://gerrit.cloudera.org:8080/#/c/24476/3/src/kudu/master/auto_leader_rebalancer.cc@708 PS3, Line 708: != This looks like a typo: moves_scheduled_this_round_for_test_ is incremented only in non-test mode. Is this as intended? -- 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: 3 Gerrit-Owner: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[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: Tue, 07 Jul 2026 17:56:45 +0000 Gerrit-HasComments: Yes
