HBASE-13350 Log warnings for sanity check failures when checks disabled.

Signed-off-by: Matteo Bertozzi <matteo.bertozzi@cloudera.com>

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 "
+      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. Set " + CONF_KEY + " to false at conf or table descriptor "
+          + "number of regions.";
-          + "if you want to bypass sanity checks");
+      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 "
+          + " very frequent flushing.";
-          + "if you want to bypass sanity checks");
+      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 "
+      String message = "Table should have at least one column family.";
-          + "Set "+CONF_KEY+" at conf or table descriptor if you want to bypass sanity checks");
+      warnOrThrowExceptionForFailure(logWarn, CONF_KEY, message, null);
     }
 
     for (HColumnDescriptor hcd : htd.getColumnFamilies()) {
       if (hcd.getTimeToLive() <= 0) {
-        throw new DoNotRetryIOException("TTL for column family " + hcd.getNameAsString()
+        String message = "TTL for column family " + hcd.getNameAsString() + " must be positive.";
-          + "  must be positive. Set " + CONF_KEY + " to false at conf or table descriptor "
+        warnOrThrowExceptionForFailure(logWarn, CONF_KEY, message, null);
-          + "if you want to bypass sanity checks");
       }
 
       // check blockSize
       if (hcd.getBlocksize() < 1024 || hcd.getBlocksize() > 16 * 1024 * 1024) {
-        throw new DoNotRetryIOException("Block size for column family " + hcd.getNameAsString()
+        String message = "Block size for column family " + hcd.getNameAsString()
-          + "  must be between 1K and 16MB Set "+CONF_KEY+" to false at conf or table descriptor "
+            + "  must be between 1K and 16MB.";
-          + "if you want to bypass sanity checks");
+        warnOrThrowExceptionForFailure(logWarn, CONF_KEY, message, null);
       }
 
       // check versions
       if (hcd.getMinVersions() < 0) {
-        throw new DoNotRetryIOException("Min versions for column family " + hcd.getNameAsString()
+        String message = "Min versions for column family " + hcd.getNameAsString()
-          + "  must be positive. Set " + CONF_KEY + " to false at conf or table descriptor "
+          + "  must be positive.";
-          + "if you want to bypass sanity checks");
+        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 "
+        String message = "Replication scope for column family "
-          + hcd.getNameAsString() + "  must be positive. Set " + CONF_KEY + " to false at conf "
+          + hcd.getNameAsString() + "  must be positive.";
-          + "or table descriptor if you want to bypass sanity checks");
+        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());

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<String> messages = new ArrayList<String>();
+
+    @Override
+    protected void append(LoggingEvent event) {
+      messages.add(event.getMessage().toString());
+    }
+
+    @Override
+    public void close() {
+    }
+
+    @Override
+    public boolean requiresLayout() {
+      return false;
+    }
+
+    public List<String> getMessages() {
+      return messages;
+    }
   }
 
   private void checkTableIsLegal(HTableDescriptor htd) throws IOException {