[
https://issues.apache.org/jira/browse/HBASE-30357?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
ASF GitHub Bot updated HBASE-30357:
-----------------------------------
Labels: pull-request-available (was: )
> OpenRegionProcedure#restoreSucceedState ignores persisted transitionCode,
> forcing OPEN even after a real FAILED_OPEN
> --------------------------------------------------------------------------------------------------------------------
>
> Key: HBASE-30357
> URL: https://issues.apache.org/jira/browse/HBASE-30357
> Project: HBase
> Issue Type: Bug
> Components: proc-v2, Region Assignment
> Reporter: Aman Poonia
> Assignee: Aman Poonia
> Priority: Major
> Labels: pull-request-available
>
> On master-failover restore, RegionRemoteProcedureBase.stateLoaded() calls
> restoreSucceedState(am, regionNode, seqId) whenever the persisted
> child-procedure
> state is REGION_REMOTE_PROCEDURE_REPORT_SUCCEED. OpenRegionProcedure's
> override:
> {code:java}
> protected void restoreSucceedState(AssignmentManager am, RegionStateNode
> regionNode,
> long openSeqNum) throws IOException {
> if (regionNode.getState() == State.OPEN) {
> return;
> }
> regionOpenedWithoutPersistingToMeta(am, regionNode,
> TransitionCode.OPENED, openSeqNum);
> }
> {code}
> unconditionally forces {{{}TransitionCode.OPENED{}}}, regardless of what the
> RegionServer
> actually reported. The real outcome (OPENED vs FAILED_OPEN) is persisted in
> the
> procedure's own {{transitionCode}} field, but the method's signature only
> receives
> {{seqId}} and structurally cannot see it.
> Concretely: if a RegionServer reports FAILED_OPEN, the master persists
> state=REPORT_SUCCEED + transitionCode=FAILED_OPEN to the procedure store, but
> does
> NOT change RegionState.State from OPENING
> ({{{}AssignmentManager#regionFailedOpen{}}} with
> {{giveUp=false}} is a no-op on state). If the master fails over before
> persistToMeta
> runs, restoreSucceedState() runs on reload, sees state=OPENING (which
> satisfies the
> guard
> {OPENING, OPEN} on regionOpenedWithoutPersistingToMeta), and force-transitions
> the region to OPEN – even though it was never actually opened on any
> RegionServer.
> {{TransitRegionStateProcedure#confirmOpened()}} has no independent check; it
> only reads
> {{{}regionNode.isInState(OPEN){}}}, the same field this bug corrupts. There
> is no rollback
> anywhere in this procedure chain (by design – forward-only), so nothing
> downstream
> can detect or correct the false OPEN once persisted to hbase:meta.
> By contrast, {{CloseRegionProcedure#restoreSucceedState}} is safe by
> construction: CLOSE
> has no FAILED_CLOSE variant at the master side (see UnassignRegionHandler.java
> comment), so forcing CLOSED on restore is always correct.
> h3. Proposed fix
> Thread the real {{transitionCode}} field through
> {{RegionRemoteProcedureBase#stateLoaded()}} into restoreSucceedState(), and
> have
> OpenRegionProcedure branch on it exactly like the live path
> ({{{}updateTransitionWithoutPersistingToMeta{}}}) already does:
> * OPENED -> regionOpenedWithoutPersistingToMeta (current behavior)
> * FAILED_OPEN -> am.regionFailedOpen(regionNode, false), letting
> TransitRegionStateProcedure#confirmOpened() retry/reassign normally.
> This only requires widening one abstract method's signature;
> CloseRegionProcedure's
> override can ignore the new parameter (no CLOSE failure variant exists).
> h3. Not a regression
> Confirmed via git archaeology: restoreSucceedState() was introduced with
> exactly this
> logic in HBASE-22365 (2019-05-10) and has been unchanged (modulo spotless
> formatting)
> through every subsequent release and backport.
> EOF)
> ⎿ On master-failover restore, RegionRemoteProcedureBase.stateLoaded() calls
> restoreSucceedState(am, regionNode, seqId) whenever the persisted
> child-procedure
> state is REGION_REMOTE_PROCEDURE_REPORT_SUCCEED. OpenRegionProcedure's
> override:
> {code:java}
> protected void restoreSucceedState(AssignmentManager am, RegionStateNode
> regionNode,
> long openSeqNum) throws IOException {
> if (regionNode.getState() == State.OPEN) {
> return;
> }
> regionOpenedWithoutPersistingToMeta(am, regionNode,
> TransitionCode.OPENED, openSeqNum);
> }
> {code}
> unconditionally forces {{{}TransitionCode.OPENED{}}}, regardless of what the
> RegionServer
> actually reported. The real outcome (OPENED vs FAILED_OPEN) is persisted in
> the
> procedure's own {{transitionCode}} field, but the method's signature only
> receives
> {{seqId}} and structurally cannot see it.
> Concretely: if a RegionServer reports FAILED_OPEN, the master persists
> state=REPORT_SUCCEED + transitionCode=FAILED_OPEN to the procedure store, but
> does
> NOT change RegionState.State from OPENING
> ({{{}AssignmentManager#regionFailedOpen{}}} with
> {{giveUp=false}} is a no-op on state). If the master fails over before
> persistToMeta
> runs, restoreSucceedState() runs on reload, sees state=OPENING (which
> satisfies the
> guard \{OPENING, OPEN}
> on regionOpenedWithoutPersistingToMeta), and force-transitions
> the region to OPEN – even though it was never actually opened on any
> RegionServer.
> {{TransitRegionStateProcedure#confirmOpened()}} has no independent check; it
> only reads
> {{{}regionNode.isInState(OPEN){}}}, the same field this bug corrupts. There
> is no rollback
> anywhere in this procedure chain (by design – forward-only), so nothing
> downstream
> can detect or correct the false OPEN once persisted to hbase:meta.
> By contrast, {{CloseRegionProcedure#restoreSucceedState}} is safe by
> construction: CLOSE
> has no FAILED_CLOSE variant at the master side (see UnassignRegionHandler.java
> comment), so forcing CLOSED on restore is always correct.
> h3. Proposed fix
> Thread the real {{transitionCode}} field through
> {{RegionRemoteProcedureBase#stateLoaded()}} into restoreSucceedState(), and
> have
> OpenRegionProcedure branch on it exactly like the live path
> ({{{}updateTransitionWithoutPersistingToMeta{}}}) already does:
> * OPENED -> regionOpenedWithoutPersistingToMeta (current behavior)
> * FAILED_OPEN -> am.regionFailedOpen(regionNode, false), letting
> TransitRegionStateProcedure#confirmOpened() retry/reassign normally.
> This only requires widening one abstract method's signature;
> CloseRegionProcedure's
> override can ignore the new parameter (no CLOSE failure variant exists).
> h3. Not a regression
> Confirmed via git archaeology: restoreSucceedState() was introduced with
> exactly this
> logic in HBASE-22365 (2019-05-10) and has been unchanged (modulo spotless
> formatting)
> through every subsequent release and backport.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
