HBASE-30291 Load backup HFile references once per cleaner run

hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupHFileCleaner.java

@@ -52,33 +52,46 @@ public class BackupHFileCleaner extends BaseHFileCleanerDelegate implements Abor
   private boolean stopped = false;
   private boolean aborted = false;
   private Connection connection;
+  // null if the references could not be loaded; in that case no files are deletable
+  private volatile Set<String> hfileFilenames;
   // timestamp of most recent completed cleaning run
   private volatile long previousCleaningCompletionTimestamp = 0;
 
   @Override
-  public void postClean() {
-    previousCleaningCompletionTimestamp = EnvironmentEdgeManager.currentTime();
-  }
-
-  @Override
-  public Iterable<FileStatus> getDeletableFiles(Iterable<FileStatus> files) {
+  public void preClean() {
     if (stopped) {
-      return Collections.emptyList();
+      hfileFilenames = null;
+      return;
     }
 
     // We use filenames because the HFile will have been moved to the archive since it
     // was registered.
-    final Set<String> hfileFilenames = new HashSet<>();
+    Set<String> filenames = new HashSet<>();
     try (BackupSystemTable tbl = new BackupSystemTable(connection)) {
       Set<TableName> tablesIncludedInBackups = fetchFullyBackedUpTables(tbl);
       for (BulkLoad bulkLoad : tbl.readBulkloadRows(tablesIncludedInBackups)) {
-        hfileFilenames.add(new Path(bulkLoad.getHfilePath()).getName());
+        filenames.add(new Path(bulkLoad.getHfilePath()).getName());
       }
-      LOG.debug("Found {} unique HFile filenames registered as bulk loads.", hfileFilenames.size());
+      LOG.debug("Found {} unique HFile filenames registered as bulk loads.", filenames.size());
+      hfileFilenames = filenames;
     } catch (IOException ioe) {
+      hfileFilenames = null;
       LOG.error(
         "Failed to read registered bulk load references from backup system table, marking all files as non-deletable.",
         ioe);
+    }
+  }
+
+  @Override
+  public void postClean() {
+    previousCleaningCompletionTimestamp = EnvironmentEdgeManager.currentTime();
+  }
+
+  @Override
+  public Iterable<FileStatus> getDeletableFiles(Iterable<FileStatus> files) {
+    // Pin the snapshot because the returned Iterable is evaluated lazily.
+    final Set<String> hfileFilenames = this.hfileFilenames;
+    if (stopped || hfileFilenames == null) {
       return Collections.emptyList();
     }
 

hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupHFileCleaner.java

@@ -24,6 +24,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.FileStatus;
 import org.apache.hadoop.fs.FileSystem;
@@ -92,23 +93,31 @@ public void testGetDeletableFiles() throws IOException {
     FileStatus file2 = createFile("file2");
     FileStatus file3 = createFile("file3");
 
+    AtomicInteger referenceLoads = new AtomicInteger();
     BackupHFileCleaner cleaner = new BackupHFileCleaner() {
       @Override
       protected Set<TableName> fetchFullyBackedUpTables(BackupSystemTable tbl) {
+        referenceLoads.incrementAndGet();
         return Set.of(tableNameWithBackup);
       }
     };
     cleaner.setConf(conf);
 
     Iterable<FileStatus> deletable;
 
-    // The first call will not allow any deletions because of the timestamp mechanism.
-    deletable = callCleaner(cleaner, List.of(file1, file1Archived, file2, file3));
+    // The first cleaning run will not allow any deletions because of the timestamp mechanism.
+    cleaner.preClean();
+    deletable = cleaner.getDeletableFiles(List.of(file1, file1Archived, file2, file3));
+    assertEquals(Set.of(), Sets.newHashSet(deletable));
+    deletable = cleaner.getDeletableFiles(List.of(file1, file1Archived, file2, file3));
     assertEquals(Set.of(), Sets.newHashSet(deletable));
+    cleaner.postClean();
+    assertEquals(1, referenceLoads.get());
 
     // No bulk loads registered, so all files can be deleted.
     deletable = callCleaner(cleaner, List.of(file1, file1Archived, file2, file3));
     assertEquals(Set.of(file1, file1Archived, file2, file3), Sets.newHashSet(deletable));
+    assertEquals(2, referenceLoads.get());
 
     // Register some bulk loads.
     try (BackupSystemTable backupSystem = new BackupSystemTable(TEST_UTIL.getConnection())) {
@@ -122,6 +131,31 @@ protected Set<TableName> fetchFullyBackedUpTables(BackupSystemTable tbl) {
     // File 1 can no longer be deleted, because it is registered as a bulk load.
     deletable = callCleaner(cleaner, List.of(file1, file1Archived, file2, file3));
     assertEquals(Set.of(file2, file3), Sets.newHashSet(deletable));
+    assertEquals(3, referenceLoads.get());
+  }
+
+  @Test
+  public void testFailedReferenceLoadKeepsFiles() throws IOException {
+    FileStatus file = createFile("file");
+    AtomicInteger referenceLoads = new AtomicInteger();
+    BackupHFileCleaner cleaner = new BackupHFileCleaner() {
+      @Override
+      protected Set<TableName> fetchFullyBackedUpTables(BackupSystemTable tbl) throws IOException {
+        if (referenceLoads.getAndIncrement() == 0) {
+          return Set.of(tableNameWithBackup);
+        }
+        throw new IOException("Failed to load references");
+      }
+    };
+    cleaner.setConf(conf);
+
+    // Complete one successful run so the file is old enough to otherwise be deletable.
+    callCleaner(cleaner, List.of(file));
+
+    cleaner.preClean();
+    Iterable<FileStatus> deletable = cleaner.getDeletableFiles(List.of(file));
+    cleaner.postClean();
+    assertEquals(Set.of(), Sets.newHashSet(deletable));
   }
 
   private Iterable<FileStatus> callCleaner(BackupHFileCleaner cleaner, Iterable<FileStatus> files) {