You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hbase.apache.org by rm...@apache.org on 2025/06/26 12:42:45 UTC
(hbase) 01/01: HBASE-29386 SnapshotProcedure and EnableTableProcedure can cause a deadlock (#7084)
This is an automated email from the ASF dual-hosted git repository.
rmattingly pushed a commit to branch HBASE-29386-backport-3
in repository https://gitbox.apache.org/repos/asf/hbase.git
commit 78a5a7345d9c90831987f59b1dca2a322bdd01a7
Author: Hernan Romer <na...@gmail.com>
AuthorDate: Thu Jun 26 08:39:25 2025 -0400
HBASE-29386 SnapshotProcedure and EnableTableProcedure can cause a deadlock (#7084)
Co-authored-by: Hernan Gelaf-Romer <hg...@hubspot.com>
Signed-off-by: Duo Zhang <zh...@apache.org>
Signed-off-by: Ray Mattingly <rm...@apache.org>
Signed-off-by: Wellington Ramos Chevreuil <wc...@apache.org>
---
.../hbase/master/procedure/SnapshotProcedure.java | 24 ---------
.../hadoop/hbase/master/procedure/TableQueue.java | 2 +-
.../TestSnapshotProcedureConcurrently.java | 57 ++++++++++++++++++++++
.../master/procedure/TestSnapshotProcedureRIT.java | 3 +-
4 files changed, 59 insertions(+), 27 deletions(-)
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SnapshotProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SnapshotProcedure.java
index 694c3878845..cc080d0da7a 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SnapshotProcedure.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SnapshotProcedure.java
@@ -104,30 +104,6 @@ public class SnapshotProcedure extends AbstractStateMachineTableProcedure<Snapsh
return TableOperationType.SNAPSHOT;
}
- @Override
- protected LockState acquireLock(MasterProcedureEnv env) {
- // AbstractStateMachineTableProcedure acquires exclusive table lock by default,
- // but we may need to downgrade it to shared lock for some reasons:
- // a. exclusive lock has a negative effect on assigning region. See HBASE-21480 for details.
- // b. we want to support taking multiple different snapshots on same table on the same time.
- if (env.getProcedureScheduler().waitTableSharedLock(this, getTableName())) {
- return LockState.LOCK_EVENT_WAIT;
- }
- return LockState.LOCK_ACQUIRED;
- }
-
- @Override
- protected void releaseLock(MasterProcedureEnv env) {
- env.getProcedureScheduler().wakeTableSharedLock(this, getTableName());
- }
-
- @Override
- protected boolean holdLock(MasterProcedureEnv env) {
- // In order to avoid enabling/disabling/modifying/deleting table during snapshot,
- // we don't release lock during suspend
- return true;
- }
-
@Override
protected Flow executeFromState(MasterProcedureEnv env, SnapshotState state)
throws ProcedureSuspendedException, ProcedureYieldException, InterruptedException {
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/TableQueue.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/TableQueue.java
index 078dc831386..be66a28d275 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/TableQueue.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/TableQueue.java
@@ -52,6 +52,7 @@ class TableQueue extends Queue<TableName> {
case CREATE:
case DELETE:
case DISABLE:
+ case SNAPSHOT:
case ENABLE:
return true;
case EDIT:
@@ -59,7 +60,6 @@ class TableQueue extends Queue<TableName> {
return !proc.getTableName().equals(TableProcedureInterface.DUMMY_NAMESPACE_TABLE_NAME);
case READ:
case FLUSH:
- case SNAPSHOT:
return false;
// region operations are using the shared-lock on the table
// and then they will grab an xlock on the region.
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestSnapshotProcedureConcurrently.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestSnapshotProcedureConcurrently.java
index b4c62aa35dc..505bc4a49a9 100644
--- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestSnapshotProcedureConcurrently.java
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestSnapshotProcedureConcurrently.java
@@ -17,6 +17,7 @@
*/
package org.apache.hadoop.hbase.master.procedure;
+import static org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProcedureProtos.EnableTableState.ENABLE_TABLE_MARK_REGIONS_ONLINE;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.fail;
@@ -27,6 +28,7 @@ import java.util.Comparator;
import java.util.List;
import java.util.stream.Collectors;
import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.TableName;
import org.apache.hadoop.hbase.client.SnapshotDescription;
import org.apache.hadoop.hbase.client.SnapshotType;
import org.apache.hadoop.hbase.master.snapshot.SnapshotManager;
@@ -136,4 +138,59 @@ public class TestSnapshotProcedureConcurrently extends TestSnapshotProcedure {
SnapshotTestingUtils.confirmSnapshotValid(TEST_UTIL, snapshotProto, TABLE_NAME, CF);
SnapshotTestingUtils.confirmSnapshotValid(TEST_UTIL, snapshotOnSameTableProto, TABLE_NAME, CF);
}
+
+ @Test
+ public void testItFailsIfTableIsNotDisabledOrEnabled() throws Exception {
+ ProcedureExecutor<MasterProcedureEnv> executor = master.getMasterProcedureExecutor();
+ MasterProcedureEnv env = executor.getEnvironment();
+ TEST_UTIL.getAdmin().disableTable(TABLE_NAME);
+
+ TestEnableTableProcedure enableTable = new TestEnableTableProcedure(
+ master.getMasterProcedureExecutor().getEnvironment(), TABLE_NAME);
+ long enableProcId = executor.submitProcedure(enableTable);
+ TEST_UTIL.waitFor(60000, () -> {
+ Procedure<MasterProcedureEnv> proc = executor.getProcedure(enableProcId);
+ if (proc == null) {
+ return false;
+ }
+ return ((TestEnableTableProcedure) proc).getProcedureState()
+ == ENABLE_TABLE_MARK_REGIONS_ONLINE;
+ });
+
+ // Using a delayed spy ensures we hit the problem state while the table enable procedure
+ // is waiting to run
+ SnapshotProcedure snapshotProc = new SnapshotProcedure(env, snapshotProto);
+ long snapshotProcId = executor.submitProcedure(snapshotProc);
+ TEST_UTIL.waitTableEnabled(TABLE_NAME);
+ // Wait for procedure to run and finish
+ TEST_UTIL.waitFor(60000, () -> executor.getProcedure(snapshotProcId) != null);
+ TEST_UTIL.waitFor(60000, () -> executor.getProcedure(snapshotProcId) == null);
+
+ SnapshotTestingUtils.confirmSnapshotValid(TEST_UTIL, snapshotProto, TABLE_NAME, CF);
+ }
+
+ // Needs to be publicly accessible for Procedure validation
+ public static class TestEnableTableProcedure extends EnableTableProcedure {
+ // Necessary for Procedure validation
+ public TestEnableTableProcedure() {
+ }
+
+ public TestEnableTableProcedure(MasterProcedureEnv env, TableName tableName) {
+ super(env, tableName);
+ }
+
+ public MasterProcedureProtos.EnableTableState getProcedureState() {
+ return getState(stateCount);
+ }
+
+ @Override
+ protected Flow executeFromState(MasterProcedureEnv env,
+ MasterProcedureProtos.EnableTableState state) throws InterruptedException {
+ if (state == ENABLE_TABLE_MARK_REGIONS_ONLINE) {
+ Thread.sleep(10000);
+ }
+
+ return super.executeFromState(env, state);
+ }
+ }
}
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestSnapshotProcedureRIT.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestSnapshotProcedureRIT.java
index 60e96f7b80d..e7cfeae324e 100644
--- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestSnapshotProcedureRIT.java
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestSnapshotProcedureRIT.java
@@ -53,8 +53,7 @@ public class TestSnapshotProcedureRIT extends TestSnapshotProcedure {
() -> procExec.getProcedure(mergeProcId).getState() == ProcedureState.RUNNABLE);
SnapshotProcedure sp = new SnapshotProcedure(procExec.getEnvironment(), snapshotProto);
long snapshotProcId = procExec.submitProcedure(sp);
- TEST_UTIL.waitFor(2000, 1000, () -> procExec.getProcedure(snapshotProcId) != null
- && procExec.getProcedure(snapshotProcId).getState() == ProcedureState.WAITING_TIMEOUT);
+ TEST_UTIL.waitFor(2000, 1000, () -> procExec.getProcedure(snapshotProcId) != null);
ProcedureTestingUtility.waitProcedure(procExec, snapshotProcId);
SnapshotTestingUtils.confirmSnapshotValid(TEST_UTIL, snapshotProto, TABLE_NAME, CF);
}