From 89a3dfb9ae6ddd84fa346c23259fe3661ca0c578 Mon Sep 17 00:00:00 2001 From: Aman Poonia Date: Sat, 5 Sep 2026 12:01:44 +0530 Subject: [PATCH] HBASE-30357 OpenRegionProcedure#restoreSucceedState ignores persisted transitionCode, forcing OPEN even after a real FAILED_OPEN On master-failover restore, RegionRemoteProcedureBase.stateLoaded() calls restoreSucceedState() for any region whose remote-open report was already persisted with state REGION_REMOTE_PROCEDURE_REPORT_SUCCEED, but before this change OpenRegionProcedure ignored the real persisted TransitionCode and always forced the region to OPEN. If the RS had actually reported FAILED_OPEN and the master crashed before persisting that to hbase:meta, the region would come back as a "phantom" OPEN region on restart: no procedure watching it, no automatic retry, and invisible to the RegionInTransition tracker since OPEN is the only non-RIT terminal state. Widen RegionRemoteProcedureBase#restoreSucceedState to also receive the persisted TransitionCode, and make OpenRegionProcedure branch on it: on FAILED_OPEN, call AssignmentManager#regionFailedOpen(regionNode, false), mirroring what the live (non-restart) reportTransition path already does. This leaves the region non-OPEN so TransitRegionStateProcedure#confirmOpened sees it and drives the normal retry loop instead of finishing silently. CloseRegionProcedure's override is updated to match the new signature but ignores the parameter, since CLOSE has no failure variant. Add TestOpenRegionProcedureRestoreFailedOpen, which reproduces the restore timing directly (invoking the same package-private stateLoaded() hook that a real master restart triggers) and asserts the region is not left in OPEN state after a FAILED_OPEN report. --- .../master/assignment/AssignmentManager.java | 9 +- .../assignment/CloseRegionProcedure.java | 5 +- .../assignment/OpenRegionProcedure.java | 7 +- .../assignment/RegionRemoteProcedureBase.java | 4 +- ...tOpenRegionProcedureRestoreFailedOpen.java | 95 +++++++++++++++++++ 5 files changed, 113 insertions(+), 7 deletions(-) create mode 100644 hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestOpenRegionProcedureRestoreFailedOpen.java diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java index 5baf30846e08..a9ae6f72a29b 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java @@ -2232,8 +2232,13 @@ CompletableFuture regionOpening(RegionStateNode regionNode) { } // should be called under the RegionStateNode lock - // The parameter 'giveUp' means whether we will try to open the region again, if it is true, then - // we will persist the FAILED_OPEN state into hbase:meta. + // Called when a region open attempt has failed. The 'giveUp' parameter says whether we will try + // to open the region again. If true, this is a terminal failure with no more retries: we set + // RegionState.State to FAILED_OPEN and persist it to hbase:meta, and only remove the region from + // its old target server's bookkeeping once that persist succeeds. If false, we are about to + // retry the open with a new plan, so RegionState.State is intentionally left untouched (e.g. + // still OPENING), meta is not touched, and we remove the region from its old target server's + // bookkeeping immediately. In both cases the removal only happens if there was an old location. CompletableFuture regionFailedOpen(RegionStateNode regionNode, boolean giveUp) { RegionState.State state = regionNode.getState(); ServerName regionLocation = regionNode.getRegionLocation(); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java index 344d7e9ad7e6..23068b3fa088 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/CloseRegionProcedure.java @@ -113,8 +113,9 @@ protected void updateTransitionWithoutPersistingToMeta(MasterProcedureEnv env, } @Override - protected void restoreSucceedState(AssignmentManager am, RegionStateNode regionNode, long seqId) - throws IOException { + protected void restoreSucceedState(AssignmentManager am, RegionStateNode regionNode, + TransitionCode transitionCode, long seqId) throws IOException { + // CLOSE has no failure variant, so transitionCode is always CLOSED here if (regionNode.getState() == State.CLOSED) { // should have already been persisted, ignore return; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/OpenRegionProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/OpenRegionProcedure.java index 21218a1177a6..8e5151f2c808 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/OpenRegionProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/OpenRegionProcedure.java @@ -127,7 +127,12 @@ protected void updateTransitionWithoutPersistingToMeta(MasterProcedureEnv env, @Override protected void restoreSucceedState(AssignmentManager am, RegionStateNode regionNode, - long openSeqNum) throws IOException { + TransitionCode transitionCode, long openSeqNum) throws IOException { + if (transitionCode == TransitionCode.FAILED_OPEN) { + // will not persist to meta if giveUp is false, matches the live reportTransition path + am.regionFailedOpen(regionNode, false); + return; + } if (regionNode.getState() == State.OPEN) { // should have already been persisted, ignore return; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionRemoteProcedureBase.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionRemoteProcedureBase.java index fe8c9bfdd7c1..dc0fb5defe2e 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionRemoteProcedureBase.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionRemoteProcedureBase.java @@ -268,12 +268,12 @@ void serverCrashed(MasterProcedureEnv env, RegionStateNode regionNode, ServerNam } protected abstract void restoreSucceedState(AssignmentManager am, RegionStateNode regionNode, - long seqId) throws IOException; + TransitionCode transitionCode, long seqId) throws IOException; void stateLoaded(AssignmentManager am, RegionStateNode regionNode) { if (state == RegionRemoteProcedureBaseState.REGION_REMOTE_PROCEDURE_REPORT_SUCCEED) { try { - restoreSucceedState(am, regionNode, seqId); + restoreSucceedState(am, regionNode, transitionCode, seqId); } catch (IOException e) { // should not happen as we are just restoring the state throw new AssertionError(e); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestOpenRegionProcedureRestoreFailedOpen.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestOpenRegionProcedureRestoreFailedOpen.java new file mode 100644 index 000000000000..aa4c34ede9fd --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestOpenRegionProcedureRestoreFailedOpen.java @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.master.assignment; + +import static org.junit.jupiter.api.Assertions.assertNotEquals; + +import java.io.IOException; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; +import org.apache.hadoop.hbase.HConstants; +import org.apache.hadoop.hbase.ServerName; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.master.RegionState; +import org.apache.hadoop.hbase.testclassification.LargeTests; +import org.apache.hadoop.hbase.testclassification.MasterTests; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; + +import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil; +import org.apache.hadoop.hbase.shaded.protobuf.generated.AdminProtos.OpenRegionRequest.RegionOpenInfo; +import org.apache.hadoop.hbase.shaded.protobuf.generated.AdminProtos.OpenRegionResponse.RegionOpeningState; +import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProtos.RegionStateTransition.TransitionCode; + +/** + * HBASE-30357: OpenRegionProcedure#restoreSucceedState() must not force the region state to OPEN on + * master-failover restore when the persisted transition code is actually FAILED_OPEN. + */ +@Tag(MasterTests.TAG) +@Tag(LargeTests.TAG) +public class TestOpenRegionProcedureRestoreFailedOpen extends TestAssignmentManagerBase { + + /** + * On the first open attempt, reports FAILED_OPEN and then immediately simulates a master restart + * happening before the child OpenRegionProcedure gets to run its own execute() (i.e. before it + * can persist anything to meta), by directly invoking the package-private stateLoaded() hook that + * a real restart would trigger. Records the region state right after that simulated restore. + * Subsequent attempts behave like a normal healthy RS so the assign procedure can converge and + * the test does not hang. + */ + private class FailedOpenThenRestoreRsExecutor extends GoodRsExecutor { + + private final AtomicBoolean firstAttempt = new AtomicBoolean(true); + + final AtomicReference stateAfterRestore = new AtomicReference<>(); + + @Override + protected RegionOpeningState execOpenRegion(ServerName server, RegionOpenInfo openReq) + throws IOException { + if (!firstAttempt.compareAndSet(true, false)) { + return super.execOpenRegion(server, openReq); + } + RegionInfo hri = ProtobufUtil.toRegionInfo(openReq.getRegion()); + RegionStateNode regionNode = am.getRegionStates().getRegionStateNode(hri); + TransitRegionStateProcedure trsp = (TransitRegionStateProcedure) regionNode.getProcedure(); + regionNode.lock(); + try { + sendTransitionReport(server, openReq.getRegion(), TransitionCode.FAILED_OPEN, + HConstants.NO_SEQNUM); + trsp.stateLoaded(am, regionNode); + stateAfterRestore.set(regionNode.getState()); + } finally { + regionNode.unlock(); + } + return RegionOpeningState.FAILED_OPENING; + } + } + + @Test + public void testRestoreDoesNotForceOpenAfterFailedOpen() throws Exception { + TableName tableName = TableName.valueOf(testMethodName); + RegionInfo hri = createRegionInfo(tableName, 1); + FailedOpenThenRestoreRsExecutor executor = new FailedOpenThenRestoreRsExecutor(); + rsDispatcher.setMockRsExecutor(executor); + TransitRegionStateProcedure proc = createAssignProcedure(hri); + waitOnFuture(submitProcedure(proc)); + + assertNotEquals(RegionState.State.OPEN, executor.stateAfterRestore.get()); + } +}