HBASE-29604 BackupHFileCleaner uses flawed time based check

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

@@ -52,10 +52,13 @@ public class BackupHFileCleaner extends BaseHFileCleanerDelegate implements Abor
   private boolean stopped = false;
   private boolean aborted = false;
   private Connection connection;
-  // timestamp of most recent read from backup system table
-  private long prevReadFromBackupTbl = 0;
-  // timestamp of 2nd most recent read from backup system table
-  private long secondPrevReadFromBackupTbl = 0;
+  // 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) {
@@ -79,12 +82,12 @@ public Iterable<FileStatus> getDeletableFiles(Iterable<FileStatus> files) {
       return Collections.emptyList();
     }
 
-    secondPrevReadFromBackupTbl = prevReadFromBackupTbl;
-    prevReadFromBackupTbl = EnvironmentEdgeManager.currentTime();
+    // Pin the threshold, we don't want the result to change depending on evaluation time.
+    final long recentFileThreshold = previousCleaningCompletionTimestamp;
 
     return Iterables.filter(files, file -> {
       // If the file is recent, be conservative and wait for one more scan of the bulk loads
-      if (file.getModificationTime() > secondPrevReadFromBackupTbl) {
+      if (file.getModificationTime() > recentFileThreshold) {
         LOG.debug("Preventing deletion due to timestamp: {}", file.getPath().toString());
         return false;
       }

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

@@ -108,11 +108,11 @@ protected Set<TableName> fetchFullyBackedUpTables(BackupSystemTable tbl) {
     Iterable<FileStatus> deletable;
 
     // The first call will not allow any deletions because of the timestamp mechanism.
-    deletable = cleaner.getDeletableFiles(List.of(file1, file1Archived, file2, file3));
+    deletable = callCleaner(cleaner, List.of(file1, file1Archived, file2, file3));
     assertEquals(Set.of(), Sets.newHashSet(deletable));
 
     // No bulk loads registered, so all files can be deleted.
-    deletable = cleaner.getDeletableFiles(List.of(file1, file1Archived, file2, file3));
+    deletable = callCleaner(cleaner, List.of(file1, file1Archived, file2, file3));
     assertEquals(Set.of(file1, file1Archived, file2, file3), Sets.newHashSet(deletable));
 
     // Register some bulk loads.
@@ -125,10 +125,17 @@ protected Set<TableName> fetchFullyBackedUpTables(BackupSystemTable tbl) {
     }
 
     // File 1 can no longer be deleted, because it is registered as a bulk load.
-    deletable = cleaner.getDeletableFiles(List.of(file1, file1Archived, file2, file3));
+    deletable = callCleaner(cleaner, List.of(file1, file1Archived, file2, file3));
     assertEquals(Set.of(file2, file3), Sets.newHashSet(deletable));
   }
 
+  private Iterable<FileStatus> callCleaner(BackupHFileCleaner cleaner, Iterable<FileStatus> files) {
+    cleaner.preClean();
+    Iterable<FileStatus> deletable = cleaner.getDeletableFiles(files);
+    cleaner.postClean();
+    return deletable;
+  }
+
   private FileStatus createFile(String fileName) throws IOException {
     Path file = new Path(root, fileName);
     fs.createNewFile(file);

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

@@ -44,6 +44,10 @@ public void init(Map<String, Object> params) {
 
   /**
    * Should the master delete the file or keep it?
+   * <p>
+   * This method can be called concurrently by multiple threads. Implementations must be thread
+   * safe.
+   * </p>
    * @param fStat file status of the file to check
    * @return <tt>true</tt> if the file is deletable, <tt>false</tt> if not
    */

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

@@ -33,6 +33,10 @@ public interface FileCleanerDelegate extends Configurable, Stoppable {
 
   /**
    * Determines which of the given files are safe to delete
+   * <p>
+   * This method can be called concurrently by multiple threads. Implementations must be thread
+   * safe.
+   * </p>
    * @param files files to check for deletion
    * @return files that are ok to delete according to this cleaner
    */