[
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:
{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.
> 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)
