tkhurana commented on code in PR #2577:
URL: https://github.com/apache/phoenix/pull/2577#discussion_r3633203086
##########
phoenix-core-server/src/main/java/org/apache/phoenix/replication/reader/ReplicationLogDiscoveryReplay.java:
##########
@@ -202,50 +228,80 @@ protected void processFile(Path path) throws IOException {
}
/**
- * Initializes lastRoundProcessed and lastRoundInSync based on HA group
state. For DEGRADED states
- * (DEGRADED_STANDBY, DEGRADED_STANDBY_FOR_WRITER): - Sets
replicationReplayState to DEGRADED -
- * Initializes lastRoundProcessed from minimum of: in-progress files, new
files, or current time -
- * Initializes lastRoundInSync from minimum of: lastSyncStateTimeInMs (from
HA Store) or minimum
- * timestamp from IN and IN PROGRESS files - This ensures lastRoundInSync
represents the last
- * known good sync point before degradation For SYNC states (STANDBY): - Sets
- * replicationReplayState to SYNC - Calls parent's
initializeLastRoundProcessed() to initialize
- * lastRoundProcessed - Sets lastRoundInSync equal to lastRoundProcessed
(both are in sync)
+ * Initializes lastRoundProcessed and lastRoundInSync based on the persisted
HA group state.
+ * <ul>
+ * <li>DEGRADED_STANDBY: sets replicationReplayState to DEGRADED and
initializes both rounds from
+ * the last known good sync point via {@link
#initLastRoundsFromLastSyncPoint(HAGroupStoreRecord)}
+ * (lastRoundInSync from the minimum of lastSyncStateTimeInMs and the file
frontier, so it
+ * represents the last consistent point before degradation).</li>
+ * <li>STANDBY_TO_ACTIVE: a restart while already mid-failover (e.g. after a
direct
+ * DEGRADED_STANDBY -> STANDBY_TO_ACTIVE transition). Initializes both
rounds from the last
+ * known good sync point, then sets replicationReplayState to
SYNCED_RECOVERY only when a rewind
+ * is actually pending (lastRoundInSync behind lastRoundProcessed) so the
first replay() rewinds
+ * before failover can promote; when the rounds are already equal it
initializes directly to SYNC
+ * so promotion is not delayed by a round window.</li>
+ * <li>Other states (e.g. STANDBY): sets replicationReplayState to SYNC,
delegates to the parent
+ * to initialize lastRoundProcessed, and sets lastRoundInSync equal to
lastRoundProcessed.</li>
+ * </ul>
+ * When the state is STANDBY_TO_ACTIVE, failoverPending is also armed.
* @throws IOException if there's an error reading HA group state or file
timestamps
*/
@Override
protected void initializeLastRoundProcessed() throws IOException {
LOG.info("Initializing last round processed for haGroup: {}", haGroupName);
+ // Sample current time BEFORE reading the HA group state, so if the group
transitions (e.g.
+ // SYNC -> DEGRADED_STANDBY) during init, the starting round still anchors
to when init began
+ // rather than to a later point after the state read and file scans.
+ long frontierStartTime = EnvironmentEdgeManager.currentTime();
HAGroupStoreRecord haGroupStoreRecord = getHAGroupRecord();
- LOG.info("Found HA Group state during initialization as {} for haGroup:
{}",
- haGroupStoreRecord.getHAGroupState(), haGroupName);
- if (
-
HAGroupStoreRecord.HAGroupState.DEGRADED_STANDBY.equals(haGroupStoreRecord.getHAGroupState())
- ) {
+ HAGroupStoreRecord.HAGroupState haGroupState =
haGroupStoreRecord.getHAGroupState();
+ LOG.info("Found HA Group state during initialization as {} for haGroup:
{}", haGroupState,
+ haGroupName);
+ // Each branch below sets the initial state with
compareAndSet(NOT_INITIALIZED, ...), not a
+ // plain set: the LOCAL state listeners are subscribed in init() before
this runs, so a
+ // concurrent LOCAL transition may have already advanced the state off
NOT_INITIALIZED. When
+ // that happens the CAS no-ops and we deliberately keep the listener's
value -- a live
+ // transition is more recent (and thus more authoritative) than the record
snapshot read above.
+ if (HAGroupStoreRecord.HAGroupState.DEGRADED_STANDBY.equals(haGroupState))
{
Review Comment:
Init-window race can leave a direct failover silently stuck (correctness /
silent failure — narrow window, self-heals on RS restart)
ReplicationLogDiscoveryReplay.java:266 interacting with
triggerFailoverListner at :178-180
Listeners are subscribed in init() before super.init() runs
initializeLastRoundProcessed(), so a LOCAL transition can fire mid-init. The
comment at :260-264 covers the
interleaving where the listener wins, but not this one:
1. getHAGroupRecord() reads (slightly stale) DEGRADED_STANDBY → picks the
DEGRADED branch.
2. Before CAS(NOT_INITIALIZED, DEGRADED) at :266, the record flips to
STANDBY_TO_ACTIVE; triggerFailoverListner runs compareAndSet(DEGRADED,
SYNCED_RECOVERY) while state
is still NOT_INITIALIZED → no-op, then failoverPending.set(true).
3. Init resumes: CAS(NOT_INITIALIZED, DEGRADED) succeeds → state DEGRADED.
4. Trailing arm-check at :316 tests the local haGroupState
(DEGRADED_STANDBY), so it doesn't reconcile.
Result: state=DEGRADED + failoverPending=true, but nothing will move
DEGRADED → SYNCED_RECOVERY (the record is already STANDBY_TO_ACTIVE and won't
re-fire; a direct
failover never revisits STANDBY). shouldTriggerFailover() gates on SYNC,
so promotion never happens until the RS restarts (the STANDBY_TO_ACTIVE init
branch heals it).
Low-probability, but it's exactly the "legitimate signal silently dropped"
case the degradedListener comment warns against.
Suggested fix: after the init branches resolve, reconcile — if
failoverPending.get() is true and resolved state is DEGRADED, promote to
SYNCED_RECOVERY (or re-derive as
the STANDBY_TO_ACTIVE branch does) and log a WARN.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
