[
https://issues.apache.org/jira/browse/HBASE-30357?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18117810#comment-18117810
]
Hudson commented on HBASE-30357:
--------------------------------
Results for branch branch-3.0
[build #37 on
builds.a.o|https://ci-hbase.apache.org/job/HBase-Integration-Test/job/branch-3.0/37/]:
(/) *{color:green}+1 overall{color}*
----
details (if available):
(/) {color:green}+1 client integration test for 3.3.5 {color}
(/) {color:green}+1 client integration test for 3.3.5 with shaded hadoop
client{color}
(/) {color:green}+1 client integration test for 3.3.6 {color}
(/) {color:green}+1 client integration test for 3.3.6 with shaded hadoop
client{color}
(/) {color:green}+1 client integration test for 3.4.0 {color}
(/) {color:green}+1 client integration test for 3.4.0 with shaded hadoop
client{color}
(/) {color:green}+1 client integration test for 3.4.1 {color}
(/) {color:green}+1 client integration test for 3.4.1 with shaded hadoop
client{color}
(/) {color:green}+1 client integration test for 3.4.2 {color}
(/) {color:green}+1 client integration test for 3.4.2 with shaded hadoop
client{color}
(/) {color:green}+1 client integration test for 3.4.3 {color}
(/) {color:green}+1 client integration test for 3.4.3 with shaded hadoop
client{color}
> 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
> Fix For: 4.0.0-alpha-1, 2.7.0, 3.1.0, 3.0.1, 2.6.8, 2.5.17
>
>
> 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)
