HBASE-13430 HFiles that are in use by a table cloned from a snapshot may be deleted when that snapshot is deleted

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

hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/HFileLinkCleaner.java

@@ -18,15 +18,15 @@
 package org.apache.hadoop.hbase.master.cleaner;
 
 import java.io.IOException;
-
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
-import org.apache.hadoop.hbase.classification.InterfaceAudience;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.FileStatus;
 import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.hbase.HBaseInterfaceAudience;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.classification.InterfaceAudience;
 import org.apache.hadoop.hbase.io.HFileLink;
 import org.apache.hadoop.hbase.util.FSUtils;
 
@@ -57,7 +57,14 @@ public class HFileLinkCleaner extends BaseHFileCleanerDelegate {
     if (HFileLink.isBackReferencesDir(parentDir)) {
       Path hfilePath = null;
       try {
-        hfilePath = HFileLink.getHFileFromBackReference(getConf(), filePath);
+        // Also check if the HFile is in the HBASE_TEMP_DIRECTORY; this is where the referenced
+        // file gets created when cloning a snapshot.
+        hfilePath = HFileLink.getHFileFromBackReference(
+            new Path(FSUtils.getRootDir(getConf()), HConstants.HBASE_TEMP_DIRECTORY), filePath);
+        if (fs.exists(hfilePath)) {
+          return false;
+        }
+        hfilePath = HFileLink.getHFileFromBackReference(FSUtils.getRootDir(getConf()), filePath);
         return !fs.exists(hfilePath);
       } catch (IOException e) {
         if (LOG.isDebugEnabled()) {

hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestSnapshotCloneIndependence.java

@@ -58,6 +58,7 @@ public class TestSnapshotCloneIndependence {
   private static final String TEST_FAM_STR = "fam";
   private static final byte[] TEST_FAM = Bytes.toBytes(TEST_FAM_STR);
   private static final TableName TABLE_NAME = TableName.valueOf(STRING_TABLE_NAME);
+  private static final int CLEANER_INTERVAL = 10;
 
   /**
    * Setup the config for the cluster and start it
@@ -87,6 +88,13 @@ public class TestSnapshotCloneIndependence {
     // Avoid potentially aggressive splitting which would cause snapshot to fail
     conf.set(HConstants.HBASE_REGION_SPLIT_POLICY_KEY,
       ConstantSizeRegionSplitPolicy.class.getName());
+    // Execute cleaner frequently to induce failures
+    conf.setInt("hbase.master.cleaner.interval", CLEANER_INTERVAL);
+    conf.setInt("hbase.master.hfilecleaner.plugins.snapshot.period", CLEANER_INTERVAL);
+    // Effectively disable TimeToLiveHFileCleaner. Don't want to fully disable it because that
+    // will even trigger races between creating the directory containing back references and
+    // the back reference itself.
+    conf.setInt("hbase.master.hfilecleaner.ttl", CLEANER_INTERVAL);
   }
 
   @Before
@@ -164,6 +172,16 @@ public class TestSnapshotCloneIndependence {
     runTestRegionOperationsIndependent(true);
   }
 
+  @Test (timeout=300000)
+  public void testOfflineSnapshotDeleteIndependent() throws Exception {
+    runTestSnapshotDeleteIndependent(false);
+  }
+
+  @Test (timeout=300000)
+  public void testOnlineSnapshotDeleteIndependent() throws Exception {
+    runTestSnapshotDeleteIndependent(true);
+  }
+
   private static void waitOnSplit(final HTable t, int originalCount) throws Exception {
     for (int i = 0; i < 200; i++) {
       try {
@@ -358,4 +376,54 @@ public class TestSnapshotCloneIndependence {
     Assert.assertTrue("The new family was not found. ",
       !clonedTableDescriptor.hasFamily(TEST_FAM_2));
   }
+
+  /*
+   * Take a snapshot of a table, add data, and verify that deleting the snapshot does not affect
+   * either table.
+   * @param online - Whether the table is online or not during the snapshot
+   */
+  private void runTestSnapshotDeleteIndependent(boolean online) throws Exception {
+    FileSystem fs = UTIL.getHBaseCluster().getMaster().getMasterFileSystem().getFileSystem();
+    Path rootDir = UTIL.getHBaseCluster().getMaster().getMasterFileSystem().getRootDir();
+
+    final Admin admin = UTIL.getHBaseAdmin();
+    final long startTime = System.currentTimeMillis();
+    final TableName localTableName =
+        TableName.valueOf(STRING_TABLE_NAME + startTime);
+
+    try (Table original = UTIL.createTable(localTableName, TEST_FAM)) {
+      UTIL.loadTable(original, TEST_FAM);
+    }
+
+    // Take a snapshot
+    final String snapshotNameAsString = "snapshot_" + localTableName;
+    byte[] snapshotName = Bytes.toBytes(snapshotNameAsString);
+
+    SnapshotTestingUtils.createSnapshotAndValidate(admin, localTableName, TEST_FAM_STR,
+        snapshotNameAsString, rootDir, fs, online);
+
+    if (!online) {
+      admin.enableTable(localTableName);
+    }
+    TableName cloneTableName = TableName.valueOf("test-clone-" + localTableName);
+    admin.cloneSnapshot(snapshotName, cloneTableName);
+
+    // Ensure the original table does not reference the HFiles anymore
+    admin.majorCompact(localTableName);
+
+    // Deleting the snapshot used to break the cloned table by deleting in-use HFiles
+    admin.deleteSnapshot(snapshotName);
+
+    // Wait for cleaner run and DFS heartbeats so that anything that is deletable is fully deleted
+    Thread.sleep(10000);
+
+    try (Table original = UTIL.getConnection().getTable(localTableName)) {
+      try (Table clonedTable = UTIL.getConnection().getTable(cloneTableName)) {
+        // Verify that all regions of both tables are readable
+        final int origTableRowCount = UTIL.countRows(original);
+        final int clonedTableRowCount = UTIL.countRows(clonedTable);
+        Assert.assertEquals(origTableRowCount, clonedTableRowCount);
+      }
+    }
+  }
 }