From a778c38ab44d84939243a440ac3b7d3845fb230d Mon Sep 17 00:00:00 2001 From: Matt Warhaftig Date: Wed, 15 Apr 2015 00:29:34 -0400 Subject: [PATCH] HBASE-13350 Log warnings for sanity check failures when checks disabled. Signed-off-by: Matteo Bertozzi --- .../apache/hadoop/hbase/master/HMaster.java | 64 +++++++++++-------- .../hbase/client/TestFromClientSide.java | 41 ++++++++++++ 2 files changed, 78 insertions(+), 27 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index 71ccf31873e..530dceadcd7 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -1405,12 +1405,13 @@ public class HMaster extends HRegionServer implements MasterServices, Server { */ private void sanityCheckTableDescriptor(final HTableDescriptor htd) throws IOException { final String CONF_KEY = "hbase.table.sanity.checks"; + boolean logWarn = false; if (!conf.getBoolean(CONF_KEY, true)) { - return; + logWarn = true; } String tableVal = htd.getConfigurationValue(CONF_KEY); if (tableVal != null && !Boolean.valueOf(tableVal)) { - return; + logWarn = true; } // check max file size @@ -1420,11 +1421,11 @@ public class HMaster extends HRegionServer implements MasterServices, Server { maxFileSize = conf.getLong(HConstants.HREGION_MAX_FILESIZE, maxFileSizeLowerLimit); } if (maxFileSize < conf.getLong("hbase.hregion.max.filesize.limit", maxFileSizeLowerLimit)) { - throw new DoNotRetryIOException("MAX_FILESIZE for table descriptor or " - + "\"hbase.hregion.max.filesize\" (" + maxFileSize - + ") is too small, which might cause over splitting into unmanageable " - + "number of regions. Set " + CONF_KEY + " to false at conf or table descriptor " - + "if you want to bypass sanity checks"); + String message = "MAX_FILESIZE for table descriptor or " + + "\"hbase.hregion.max.filesize\" (" + maxFileSize + + ") is too small, which might cause over splitting into unmanageable " + + "number of regions."; + warnOrThrowExceptionForFailure(logWarn, CONF_KEY, message, null); } // check flush size @@ -1434,72 +1435,81 @@ public class HMaster extends HRegionServer implements MasterServices, Server { flushSize = conf.getLong(HConstants.HREGION_MEMSTORE_FLUSH_SIZE, flushSizeLowerLimit); } if (flushSize < conf.getLong("hbase.hregion.memstore.flush.size.limit", flushSizeLowerLimit)) { - throw new DoNotRetryIOException("MEMSTORE_FLUSHSIZE for table descriptor or " + String message = "MEMSTORE_FLUSHSIZE for table descriptor or " + "\"hbase.hregion.memstore.flush.size\" ("+flushSize+") is too small, which might cause" - + " very frequent flushing. Set " + CONF_KEY + " to false at conf or table descriptor " - + "if you want to bypass sanity checks"); + + " very frequent flushing."; + warnOrThrowExceptionForFailure(logWarn, CONF_KEY, message, null); } // check that coprocessors and other specified plugin classes can be loaded try { checkClassLoading(conf, htd); } catch (Exception ex) { - throw new DoNotRetryIOException(ex); + warnOrThrowExceptionForFailure(logWarn, CONF_KEY, ex.getMessage(), null); } // check compression can be loaded try { checkCompression(htd); } catch (IOException e) { - throw new DoNotRetryIOException(e.getMessage(), e); + warnOrThrowExceptionForFailure(logWarn, CONF_KEY, e.getMessage(), e); } // check encryption can be loaded try { checkEncryption(conf, htd); } catch (IOException e) { - throw new DoNotRetryIOException(e.getMessage(), e); + warnOrThrowExceptionForFailure(logWarn, CONF_KEY, e.getMessage(), e); } // check that we have at least 1 CF if (htd.getColumnFamilies().length == 0) { - throw new DoNotRetryIOException("Table should have at least one column family " - + "Set "+CONF_KEY+" at conf or table descriptor if you want to bypass sanity checks"); + String message = "Table should have at least one column family."; + warnOrThrowExceptionForFailure(logWarn, CONF_KEY, message, null); } for (HColumnDescriptor hcd : htd.getColumnFamilies()) { if (hcd.getTimeToLive() <= 0) { - throw new DoNotRetryIOException("TTL for column family " + hcd.getNameAsString() - + " must be positive. Set " + CONF_KEY + " to false at conf or table descriptor " - + "if you want to bypass sanity checks"); + String message = "TTL for column family " + hcd.getNameAsString() + " must be positive."; + warnOrThrowExceptionForFailure(logWarn, CONF_KEY, message, null); } // check blockSize if (hcd.getBlocksize() < 1024 || hcd.getBlocksize() > 16 * 1024 * 1024) { - throw new DoNotRetryIOException("Block size for column family " + hcd.getNameAsString() - + " must be between 1K and 16MB Set "+CONF_KEY+" to false at conf or table descriptor " - + "if you want to bypass sanity checks"); + String message = "Block size for column family " + hcd.getNameAsString() + + " must be between 1K and 16MB."; + warnOrThrowExceptionForFailure(logWarn, CONF_KEY, message, null); } // check versions if (hcd.getMinVersions() < 0) { - throw new DoNotRetryIOException("Min versions for column family " + hcd.getNameAsString() - + " must be positive. Set " + CONF_KEY + " to false at conf or table descriptor " - + "if you want to bypass sanity checks"); + String message = "Min versions for column family " + hcd.getNameAsString() + + " must be positive."; + warnOrThrowExceptionForFailure(logWarn, CONF_KEY, message, null); } // max versions already being checked // check replication scope if (hcd.getScope() < 0) { - throw new DoNotRetryIOException("Replication scope for column family " - + hcd.getNameAsString() + " must be positive. Set " + CONF_KEY + " to false at conf " - + "or table descriptor if you want to bypass sanity checks"); + String message = "Replication scope for column family " + + hcd.getNameAsString() + " must be positive."; + warnOrThrowExceptionForFailure(logWarn, CONF_KEY, message, null); } // TODO: should we check coprocessors and encryption ? } } + // HBASE-13350 - Helper method to log warning on sanity check failures if checks disabled. + private static void warnOrThrowExceptionForFailure(boolean logWarn, String confKey, + String message, Exception cause) throws IOException { + if (!logWarn) { + throw new DoNotRetryIOException(message + " Set " + confKey + + " to false at conf or table descriptor if you want to bypass sanity checks", cause); + } + LOG.warn(message); + } + private void startActiveMasterManager(int infoPort) throws KeeperException { String backupZNode = ZKUtil.joinZNode( zooKeeper.backupMasterAddressesZNode, serverName.toString()); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java index ec45923eeb8..29607ea30a2 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java @@ -43,6 +43,7 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.atomic.AtomicReference; +import org.apache.log4j.Level; import org.apache.commons.lang.ArrayUtils; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -82,6 +83,7 @@ import org.apache.hadoop.hbase.filter.WhileMatchFilter; import org.apache.hadoop.hbase.io.hfile.BlockCache; import org.apache.hadoop.hbase.io.hfile.CacheConfig; import org.apache.hadoop.hbase.ipc.CoprocessorRpcChannel; +import org.apache.hadoop.hbase.master.HMaster; import org.apache.hadoop.hbase.protobuf.ProtobufUtil; import org.apache.hadoop.hbase.protobuf.generated.AdminProtos; import org.apache.hadoop.hbase.protobuf.generated.ClientProtos.MutationProto; @@ -98,6 +100,9 @@ import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.hbase.util.EnvironmentEdgeManager; import org.apache.hadoop.hbase.util.Pair; import org.apache.hadoop.hbase.zookeeper.ZooKeeperWatcher; +import org.apache.log4j.AppenderSkeleton; +import org.apache.log4j.Logger; +import org.apache.log4j.spi.LoggingEvent; import org.junit.After; import org.junit.AfterClass; import org.junit.Before; @@ -5535,8 +5540,44 @@ public class TestFromClientSide { // check the conf settings to disable sanity checks htd.setMemStoreFlushSize(0); + + // Check that logs warn on invalid table but allow it. + ListAppender listAppender = new ListAppender(); + Logger log = Logger.getLogger(HMaster.class); + log.addAppender(listAppender); + log.setLevel(Level.WARN); + htd.setConfiguration("hbase.table.sanity.checks", Boolean.FALSE.toString()); checkTableIsLegal(htd); + + assertFalse(listAppender.getMessages().isEmpty()); + assertTrue(listAppender.getMessages().get(0).startsWith("MEMSTORE_FLUSHSIZE for table " + + "descriptor or \"hbase.hregion.memstore.flush.size\" (0) is too small, which might " + + "cause very frequent flushing.")); + + log.removeAppender(listAppender); + } + + private static class ListAppender extends AppenderSkeleton { + private final List messages = new ArrayList(); + + @Override + protected void append(LoggingEvent event) { + messages.add(event.getMessage().toString()); + } + + @Override + public void close() { + } + + @Override + public boolean requiresLayout() { + return false; + } + + public List getMessages() { + return messages; + } } private void checkTableIsLegal(HTableDescriptor htd) throws IOException {