PHOENIX-7990 Reconcile convergent CAS race in setHAGroupStatusIfNeeded to avoid RS aborts - #2604
Open
tkhurana wants to merge 1 commit into
Open
PHOENIX-7990 Reconcile convergent CAS race in setHAGroupStatusIfNeeded to avoid RS aborts#2604tkhurana wants to merge 1 commit into
tkhurana wants to merge 1 commit into
Conversation
…d to avoid RS aborts When >=2 co-active RegionServers drive the shared HA-status znode to the same target state, each does an optimistic CAS whose expected version comes from the watch-lagged local cache. The first writer wins; the others were aborting the RegionServer even though the shared status was already at the target. This hits the peer-degrade path (ACTIVE_IN_SYNC -> ACTIVE_NOT_IN_SYNC) and, more commonly, the forwarders racing ACTIVE_NOT_IN_SYNC -> ACTIVE_IN_SYNC. The two convergent losers differ by whether the winner's write reached the loser's cache before it fired, and each is reconciled from a source that is authoritative for its path -- never the watch-lagged cache used as a false-success signal: Cache-stale loser (client, HAGroupStoreClient.setHAGroupStatusIfNeeded): the CAS is attempted and loses stale; the loop re-reads fresh from ZK and retries, bounded by SET_HA_GROUP_STATUS_MAX_ATTEMPTS. Move the existing 'attempt > 1 && current == target' no-op ahead of validateTransitionAndGetWaitTime so a converged non-self-transitionable target (e.g. ACTIVE_IN_SYNC) returns a no-op success from the fresh re-read instead of throwing InvalidClusterRoleTransitionException on the X -> X self-transition. The first attempt never short-circuits, so the periodic STORE_AND_FORWARD heartbeat still writes its znode mtime bump that gates SYNC promotion. Watch-won loser (server, HAGroupStoreManager.setHAGroupStatusToSync): the winner's write already propagated to the cache, so validate rejects the ACTIVE_IN_SYNC -> ACTIVE_IN_SYNC self-transition before any CAS is attempted -- no stale-version exception is ever thrown, so the client cannot catch it. Swallow the InvalidClusterRoleTransitionException as a no-op success, guarded on isStateAlreadyUpdated(current == target) so a genuinely invalid transition still propagates. The ACTIVE_IN_SYNC local listener in ReplicationLogGroup already drives the loser to the correct end state (resume rotation, SYNC_AND_FORWARD -> SYNC), so the group-level goal is met and the loser need not retry its own write. Add HAGroupStoreManagerIT#testSetHAGroupStatusToSyncConvergentRaceIsNoOp covering the watch-won path: with the record already at ACTIVE_IN_SYNC, setHAGroupStatusToSync returns 0L with no ZK write instead of throwing. The existing HAGroupStoreClientIT convergent-race and heartbeat cases continue to cover the cache-stale path. Follow-up (review): add HAGroupStoreClientIT# testSetHAGroupStatusIfNeededConvergentRaceNonSelfTransitionableTarget, a subcase-A regression that converges on a non-self-transitionable target (STANDBY -> STANDBY_TO_ACTIVE): the existing convergent test converges on ACTIVE_NOT_IN_SYNC (the lone self-transitionable state) and so passes even without the reorder, whereas this one throws InvalidClusterRoleTransitionException if the no-op short-circuit is not moved ahead of validate. Also raise the ReplicationLogDiscoveryForwarder swallowed setHAGroupStatusToSync failure from INFO to WARN: convergent races now reconcile to a no-op success, so a throw there is a genuine anomaly (retry-exhausted contention or a truly invalid transition) and must be visible.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When >=2 co-active RegionServers drive the shared HA-status znode to the same target state, each does an optimistic CAS whose expected version comes from the watch-lagged local cache. The first writer wins; the others were aborting the RegionServer even though the shared status was already at the target. This hits the peer-degrade path (ACTIVE_IN_SYNC -> ACTIVE_NOT_IN_SYNC) and, more commonly, the forwarders racing ACTIVE_NOT_IN_SYNC -> ACTIVE_IN_SYNC.
The two convergent losers differ by whether the winner's write reached the loser's cache before it fired, and each is reconciled from a source that is authoritative for its path -- never the watch-lagged cache used as a false-success signal:
Cache-stale loser (client, HAGroupStoreClient.setHAGroupStatusIfNeeded): the CAS is attempted and loses stale; the loop re-reads fresh from ZK and retries, bounded by SET_HA_GROUP_STATUS_MAX_ATTEMPTS. Move 'attempt > 1 && current == target' no-op ahead of validateTransitionAndGetWaitTime so a converged non-self-transitionable target (e.g. ACTIVE_IN_SYNC) returns a no-op success from the fresh re-read instead of throwing InvalidClusterRoleTransitionException on the X -> X self-transition. The first attempt never short-circuits, so the periodic STORE_AND_FORWARD heartbeat still
writes its znode mtime bump that gates SYNC promotion.
Watch-won loser (server, HAGroupStoreManager.setHAGroupStatusToSync): the winner's write already propagated to the cache, so validate rejects the ACTIVE_IN_SYNC -> ACTIVE_IN_SYNC self-transition before any CAS is attempted -- no stale-version exception is ever thrown, so the client cannot catch it. Swallow the InvalidClusterRoleTransitionException as a no-op success, guarded on isStateAlreadyUpdated(current == target) so a genuinely invalid transition still propagates. The ACTIVE_IN_SYNC local listener in ReplicationLogGroup already drives the loser to the correct end state (resume rotation, SYNC_AND_FORWARD -> SYNC), so the group-level goal is met and the loser need not retry its own write.