diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupHFileCleaner.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupHFileCleaner.java index bbbae2d631fe..2b98a5065445 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupHFileCleaner.java +++ b/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 hfileFilenames; // timestamp of most recent completed cleaning run private volatile long previousCleaningCompletionTimestamp = 0; @Override - public void postClean() { - previousCleaningCompletionTimestamp = EnvironmentEdgeManager.currentTime(); - } - - @Override - public Iterable getDeletableFiles(Iterable 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 hfileFilenames = new HashSet<>(); + Set filenames = new HashSet<>(); try (BackupSystemTable tbl = new BackupSystemTable(connection)) { Set 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 getDeletableFiles(Iterable files) { + // Pin the snapshot because the returned Iterable is evaluated lazily. + final Set hfileFilenames = this.hfileFilenames; + if (stopped || hfileFilenames == null) { return Collections.emptyList(); } diff --git a/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupHFileCleaner.java b/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupHFileCleaner.java index 02c48e8e11fd..36c5458731dd 100644 --- a/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupHFileCleaner.java +++ b/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,9 +93,11 @@ public void testGetDeletableFiles() throws IOException { FileStatus file2 = createFile("file2"); FileStatus file3 = createFile("file3"); + AtomicInteger referenceLoads = new AtomicInteger(); BackupHFileCleaner cleaner = new BackupHFileCleaner() { @Override protected Set fetchFullyBackedUpTables(BackupSystemTable tbl) { + referenceLoads.incrementAndGet(); return Set.of(tableNameWithBackup); } }; @@ -102,13 +105,19 @@ protected Set fetchFullyBackedUpTables(BackupSystemTable tbl) { Iterable 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 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 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 deletable = cleaner.getDeletableFiles(List.of(file)); + cleaner.postClean(); + assertEquals(Set.of(), Sets.newHashSet(deletable)); } private Iterable callCleaner(BackupHFileCleaner cleaner, Iterable files) {