diff --git a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java index fb3b9d90bf75..0a3c43b6790b 100644 --- a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java +++ b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java @@ -1480,7 +1480,7 @@ private void executeProcedure(Procedure 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 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 proc) { + private void rootProcedureFinished(Procedure proc) { // call the procedure completion cleanup handler execCompletionCleanup(proc); @@ -2046,14 +2050,6 @@ private void procedureFinished(Procedure 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()); } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestTableProcedureWaitingQueueCleanup.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestTableProcedureWaitingQueueCleanup.java new file mode 100644 index 000000000000..704719d43655 --- /dev/null +++ b/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 + implements PeerProcedureInterface { + + private boolean created = false; + + @Override + protected Procedure[] 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 + 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[] 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 proc) throws Exception { + ProcedureExecutor 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()); + } +}