[
https://issues.apache.org/jira/browse/HBASE-30357?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Aman Poonia updated HBASE-30357:
--------------------------------
Description:
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.
was:
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:
protected void restoreSucceedState(AssignmentManager am, RegionStateNode
regionNode,
long openSeqNum) throws IOException {
if (regionNode.getState() == State.OPEN) {
return;
}
regionOpenedWithoutPersistingToMeta(am, regionNode,
TransitionCode.OPENED, openSeqNum);
}
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.
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).
> 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
>
> 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)
