Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/24285 )
Change subject: KUDU-3732 Get consensus state from CatalogManager in auto-rebalancer ...................................................................... Patch Set 4: (3 comments) Some questions from my end. Can you please add tests that cover the core behaviour of this patch? Thanks! http://gerrit.cloudera.org:8080/#/c/24285/4/src/kudu/master/auto_rebalancer.cc File src/kudu/master/auto_rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/24285/4/src/kudu/master/auto_rebalancer.cc@504 PS4, Line 504: { : CatalogManager::ScopedLeaderSharedLock l(catalog_manager_); : RETURN_NOT_OK(l.first_failed_status()); : RETURN_NOT_OK(catalog_manager_->GetTabletConsensusState(tablet_id, &pre_cstate)); : } : const int64_t pre_opid_index = pre_cstate.committed_config().opid_index(); : : string leader_uuid; : HostPort leader_hp; : RETURN_NOT_OK(GetTabletLeader(tablet_id, &leader_uuid, &leader_hp)); GetTabletConsensusState (line 504-508) and GetTabletLeader (line 513) each acquire ScopedLeaderSharedLock independently. Between the two reads the catalog could advance. This means pre_opid_index could be stale relative to the actual leader's committed index by the time BulkChangeConfig is sent. In that case the CAS check will fail on the leader, the move is logged as a scheduling failure and retried next cycle. This is correct behavior, but in a busy cluster where the catalog consistently lags the leader by one heartbeat interval every round could see spurious CAS rejections. Worth adding a comment (or a TODO) noting this trade-off - especially since it replaces the old path that read directly from the leader and thus never had stale-index CAS failures. http://gerrit.cloudera.org:8080/#/c/24285/4/src/kudu/master/auto_rebalancer.cc@549 PS4, Line 549: pre_opid_index + 1 nit: should this be pre_opid_index + N (N > 1) if there are non-config entries in between? http://gerrit.cloudera.org:8080/#/c/24285/4/src/kudu/master/auto_rebalancer.cc@931 PS4, Line 931: if (replica_move.config_opid_idx && If the DCHECK above is correct, the `&&` in the if is redundant in debug builds and misleading in release builds. In release, if config_opid_idx is somehow absent, the `&&` short-circuits the whole condition to false. The staleness gate is silently bypassed and CheckMoveCompleted proceeds to inspect potentially stale CatalogManager state. -- To view, visit http://gerrit.cloudera.org:8080/24285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba021539fbfaca5905fbbf4abbb6e2a61db1f7dc Gerrit-Change-Number: 24285 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: Thu, 28 May 2026 11:04:06 +0000 Gerrit-HasComments: Yes
