HBASE-28876 Should call ProcedureSchduler.completionCleanup for non-r…

hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java

@@ -1480,7 +1480,7 @@ private void executeProcedure(Procedure<TEnvironment> proc) {
         LOG.info("Finished " + proc + " in " + StringUtils.humanTimeDiff(proc.elapsedTime()));
         // Finalize the procedure state
         if (proc.getProcId() == rootProcId) {
-          procedureFinished(proc);
+          rootProcedureFinished(proc);
         } else {
           execCompletionCleanup(proc);
         }
@@ -1665,7 +1665,7 @@ private LockState executeRollback(long rootProcId, RootProcedureState<TEnvironme
       // Finalize the procedure state
       LOG.info("Rolled back {} exec-time={}", rootProc,
         StringUtils.humanTimeDiff(rootProc.elapsedTime()));
-      procedureFinished(rootProc);
+      rootProcedureFinished(rootProc);
     } finally {
       if (lockEntry != null) {
         procExecutionLock.releaseLockEntry(lockEntry);
@@ -2027,11 +2027,15 @@ private void execCompletionCleanup(Procedure<TEnvironment> proc) {
       proc.completionCleanup(env);
     } catch (Throwable e) {
       // Catch NullPointerExceptions or similar errors...
-      LOG.error("CODE-BUG: uncatched runtime exception for procedure: " + proc, e);
+      LOG.error("CODE-BUG: uncaught runtime exception for procedure: " + proc, e);
     }
+
+    // call schedulers completion cleanup, we have some special clean up logic in this method so if
+    // it throws any exceptions, we can not just ignore it like the above procedure's cleanup
+    scheduler.completionCleanup(proc);
   }
 
-  private void procedureFinished(Procedure<TEnvironment> proc) {
+  private void rootProcedureFinished(Procedure<TEnvironment> proc) {
     // call the procedure completion cleanup handler
     execCompletionCleanup(proc);
 
@@ -2046,14 +2050,6 @@ private void procedureFinished(Procedure<TEnvironment> proc) {
     rollbackStack.remove(proc.getProcId());
     procedures.remove(proc.getProcId());
 
-    // call the runnableSet completion cleanup handler
-    try {
-      scheduler.completionCleanup(proc);
-    } catch (Throwable e) {
-      // Catch NullPointerExceptions or similar errors...
-      LOG.error("CODE-BUG: uncatched runtime exception for completion cleanup: {}", proc, e);
-    }
-
     // Notify the listeners
     sendProcedureFinishedNotification(proc.getProcId());
   }

hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestTableProcedureWaitingQueueCleanup.java

@@ -0,0 +1,184 @@
+/*
+ * 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.procedure;
+
+import static org.junit.Assert.assertTrue;
+
+import java.io.IOException;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.HBaseTestingUtility;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.client.RegionInfoBuilder;
+import org.apache.hadoop.hbase.client.TableDescriptor;
+import org.apache.hadoop.hbase.client.TableDescriptorBuilder;
+import org.apache.hadoop.hbase.procedure2.Procedure;
+import org.apache.hadoop.hbase.procedure2.ProcedureExecutor;
+import org.apache.hadoop.hbase.procedure2.ProcedureStateSerializer;
+import org.apache.hadoop.hbase.procedure2.ProcedureSuspendedException;
+import org.apache.hadoop.hbase.procedure2.ProcedureTestingUtility;
+import org.apache.hadoop.hbase.procedure2.ProcedureYieldException;
+import org.apache.hadoop.hbase.testclassification.MasterTests;
+import org.apache.hadoop.hbase.testclassification.MediumTests;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+/**
+ * Testcase for HBASE-28876
+ */
+@Category({ MasterTests.class, MediumTests.class })
+public class TestTableProcedureWaitingQueueCleanup {
+
+  @ClassRule
+  public static final HBaseClassTestRule CLASS_RULE =
+    HBaseClassTestRule.forClass(TestTableProcedureWaitingQueueCleanup.class);
+
+  private static final HBaseTestingUtility UTIL = new HBaseTestingUtility();
+
+  private static TableDescriptor TD = TableDescriptorBuilder.newBuilder(TableName.valueOf("test"))
+    .setColumnFamily(ColumnFamilyDescriptorBuilder.of("cf")).build();
+
+  // In current HBase code base, we do not use table procedure as sub procedure, so here we need to
+  // introduce one for testing
+  public static class NonTableProcedure extends Procedure<MasterProcedureEnv>
+    implements PeerProcedureInterface {
+
+    private boolean created = false;
+
+    @Override
+    protected Procedure<MasterProcedureEnv>[] execute(MasterProcedureEnv env)
+      throws ProcedureYieldException, ProcedureSuspendedException, InterruptedException {
+      if (created) {
+        return null;
+      }
+      created = true;
+      return new Procedure[] { new CreateTableProcedure(env, TD,
+        new RegionInfo[] { RegionInfoBuilder.newBuilder(TD.getTableName()).build() }) };
+    }
+
+    @Override
+    protected void rollback(MasterProcedureEnv env) throws IOException, InterruptedException {
+      throw new UnsupportedOperationException();
+    }
+
+    @Override
+    protected boolean abort(MasterProcedureEnv env) {
+      return false;
+    }
+
+    @Override
+    protected void serializeStateData(ProcedureStateSerializer serializer) throws IOException {
+    }
+
+    @Override
+    protected void deserializeStateData(ProcedureStateSerializer serializer) throws IOException {
+    }
+
+    @Override
+    public String getPeerId() {
+      return "peer";
+    }
+
+    @Override
+    public PeerOperationType getPeerOperationType() {
+      return PeerOperationType.ENABLE;
+    }
+  }
+
+  @BeforeClass
+  public static void setUp() throws Exception {
+    UTIL.startMiniCluster();
+  }
+
+  @AfterClass
+  public static void tearDown() throws Exception {
+    UTIL.shutdownMiniCluster();
+  }
+
+  // the root procedure will lock meta but we will schedule a table procedure for other table
+  public static class MetaTableProcedure extends Procedure<MasterProcedureEnv>
+    implements TableProcedureInterface {
+
+    private boolean created = false;
+
+    @Override
+    public TableName getTableName() {
+      return TableName.META_TABLE_NAME;
+    }
+
+    @Override
+    public TableOperationType getTableOperationType() {
+      return TableOperationType.EDIT;
+    }
+
+    @Override
+    protected Procedure<MasterProcedureEnv>[] execute(MasterProcedureEnv env)
+      throws ProcedureYieldException, ProcedureSuspendedException, InterruptedException {
+      if (created) {
+        return null;
+      }
+      created = true;
+      return new Procedure[] { new CreateTableProcedure(env, TD,
+        new RegionInfo[] { RegionInfoBuilder.newBuilder(TD.getTableName()).build() }) };
+    }
+
+    @Override
+    protected void rollback(MasterProcedureEnv env) throws IOException, InterruptedException {
+      throw new UnsupportedOperationException();
+    }
+
+    @Override
+    protected boolean abort(MasterProcedureEnv env) {
+      return false;
+    }
+
+    @Override
+    protected void serializeStateData(ProcedureStateSerializer serializer) throws IOException {
+    }
+
+    @Override
+    protected void deserializeStateData(ProcedureStateSerializer serializer) throws IOException {
+    }
+  }
+
+  private void testCreateDelete(Procedure<MasterProcedureEnv> proc) throws Exception {
+    ProcedureExecutor<MasterProcedureEnv> procExec =
+      UTIL.getMiniHBaseCluster().getMaster().getMasterProcedureExecutor();
+    ProcedureTestingUtility.submitAndWait(procExec, proc);
+    assertTrue(UTIL.getAdmin().tableExists(TD.getTableName()));
+
+    // Without the fix in HBASE-28876, we will hang there forever, as we do not clean up the
+    // TableProcedureWaitingQueue
+    UTIL.getAdmin().disableTable(TD.getTableName());
+    UTIL.getAdmin().deleteTable(TD.getTableName());
+  }
+
+  @Test
+  public void testNonTableProcedure() throws Exception {
+    testCreateDelete(new NonTableProcedure());
+  }
+
+  @Test
+  public void testNotSameTable() throws Exception {
+    testCreateDelete(new MetaTableProcedure());
+  }
+}