From c9950b5a794946954636e1115ad5afc17e8ef555 Mon Sep 17 00:00:00 2001 From: Thiruvel Thirumoolan Date: Fri, 12 Jan 2018 11:26:48 -0800 Subject: [PATCH] HBASE-19756 Master NPE during completed failed proc eviction Signed-off-by: Andrew Purtell --- .../hbase/procedure2/ProcedureExecutor.java | 20 ++-- .../procedure/TestFailedProcCleanup.java | 110 ++++++++++++++++++ 2 files changed, 121 insertions(+), 9 deletions(-) create mode 100644 hbase-server/src/test/java/org/apache/hadoop/hbase/procedure/TestFailedProcCleanup.java 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 2db8d321a55..4a612998b30 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 @@ -217,20 +217,22 @@ public class ProcedureExecutor { // TODO: Select TTL based on Procedure type if (retainer.isExpired(now, evictTtl, evictAckTtl)) { - if (debugEnabled) { - LOG.debug("Evict completed " + proc); + // Failed procedures aren't persisted in WAL. + if (!(proc instanceof FailedProcedure)) { + batchIds[batchCount++] = entry.getKey(); + if (batchCount == batchIds.length) { + store.delete(batchIds, 0, batchCount); + batchCount = 0; + } } - batchIds[batchCount++] = entry.getKey(); - if (batchCount == batchIds.length) { - store.delete(batchIds, 0, batchCount); - batchCount = 0; - } - it.remove(); - final NonceKey nonceKey = proc.getNonceKey(); if (nonceKey != null) { nonceKeysToProcIdsMap.remove(nonceKey); } + it.remove(); + if (debugEnabled) { + LOG.debug("Evict completed " + proc); + } } } if (batchCount > 0) { diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/procedure/TestFailedProcCleanup.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/procedure/TestFailedProcCleanup.java new file mode 100644 index 00000000000..740caea09d3 --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/procedure/TestFailedProcCleanup.java @@ -0,0 +1,110 @@ +/* + * 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.procedure; + +import static org.junit.Assert.fail; + +import java.io.IOException; +import java.util.List; +import java.util.Optional; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.HBaseTestingUtility; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.client.TableDescriptor; +import org.apache.hadoop.hbase.coprocessor.CoprocessorHost; +import org.apache.hadoop.hbase.coprocessor.MasterCoprocessor; +import org.apache.hadoop.hbase.coprocessor.MasterCoprocessorEnvironment; +import org.apache.hadoop.hbase.coprocessor.MasterObserver; +import org.apache.hadoop.hbase.coprocessor.ObserverContext; +import org.apache.hadoop.hbase.procedure2.Procedure; +import org.apache.hadoop.hbase.security.AccessDeniedException; +import org.apache.hadoop.hbase.shaded.protobuf.generated.ProcedureProtos; +import org.apache.hadoop.hbase.testclassification.MediumTests; +import org.apache.hadoop.hbase.util.Bytes; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +/** + * Check if CompletedProcedureCleaner cleans up failed nonce procedures. + */ +@Category(MediumTests.class) +public class TestFailedProcCleanup { + private static final Log LOG = LogFactory.getLog(TestFailedProcCleanup.class); + + protected static HBaseTestingUtility TEST_UTIL = new HBaseTestingUtility(); + private static final TableName TABLE = TableName.valueOf("test"); + private static final byte[] FAMILY = Bytes.toBytesBinary("f"); + private static final int evictionDelay = 10 * 1000; + + @BeforeClass + public static void setUpBeforeClass() throws Exception { + Configuration conf = TEST_UTIL.getConfiguration(); + conf.setInt("hbase.procedure.cleaner.evict.ttl", evictionDelay); + conf.setInt("hbase.procedure.cleaner.evict.batch.size", 1); + conf.set(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, CreateFailObserver.class.getName()); + TEST_UTIL.startMiniCluster(3); + } + + @AfterClass + public static void tearDownAfterClass() throws Exception { + TEST_UTIL.cleanupTestDir(); + TEST_UTIL.cleanupDataTestDirOnTestFS(); + TEST_UTIL.shutdownMiniCluster(); + } + + @Test + public void testFailCreateTable() throws Exception { + try { + TEST_UTIL.createTable(TABLE, FAMILY); + fail("Table shouldn't be created"); + } catch (AccessDeniedException e) { + LOG.debug("Ignoring exception: ", e); + Thread.sleep(evictionDelay * 3); + } + List> procedureInfos = + TEST_UTIL.getMiniHBaseCluster().getMaster().getMasterProcedureExecutor().getProcedures(); + for (Procedure procedureInfo : procedureInfos) { + if (procedureInfo.getProcName().equals("CreateTableProcedure") + && procedureInfo.getState() == ProcedureProtos.ProcedureState.ROLLEDBACK) { + fail("Found procedure " + procedureInfo + " that hasn't been cleaned up"); + } + } + } + + public static class CreateFailObserver implements MasterCoprocessor, MasterObserver { + + @Override + public void preCreateTable(ObserverContext env, + TableDescriptor desc, RegionInfo[] regions) throws IOException { + + if (desc.getTableName().equals(TABLE)) { + throw new AccessDeniedException("Don't allow creation of table"); + } + } + + @Override + public Optional getMasterObserver() { + return Optional.of(this); + } + } +}