HBASE-29905 BackupLogCleaner retains old WAL files due to stale entries in system:backup table

janvanbesien-ngdata · Dieter De Paepe · commit 1f676bbb04bc · 2026-04-03T20:06:55.000+02:00
BackupLogCleaner.serverToPreservationBoundaryTs() computes the WAL
deletion boundary by iterating over tableSetTimestampMap from persisted
BackupInfo sessions. This map can contain entries for tables that were
once part of the backup set but have since been deleted (or had all
their backups deleted). Their stale, old timestamps drag the minimum WAL
boundary back, preventing old WALs from being cleaned up.

Fix: when computing boundaries, load the incrbackupset per backup root
from BackupSystemTable and skip tables not in the active set. The
incrbackupset is populated on every full backup and pruned when backups
are deleted, so it accurately reflects which tables still need WAL
retention.
diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java
@@ -25,11 +25,13 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.stream.Collectors;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.FileStatus;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.hbase.HBaseInterfaceAudience;
+import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.backup.BackupInfo;
 import org.apache.hadoop.hbase.backup.BackupInfo.BackupState;
 import org.apache.hadoop.hbase.backup.BackupRestoreConstants;
@@ -89,11 +91,13 @@ public void init(Map<String, Object> params) {
    * Calculates the timestamp boundary up to which all backup roots have already included the WAL.
    * I.e. WALs with a lower (= older) or equal timestamp are no longer needed for future incremental
    * backups.
-   * @param backups  all completed or running backups to use for the calculation of the boundary
-   * @param tsBuffer a buffer (in ms) to lower the boundary for the default bound
+   * @param backups                   all completed or running backups to use for the calculation of
+   *                                  the boundary
+   * @param incrBackupTableSetPerRoot per backup root, the tables included in incremental backups
+   * @param tsBuffer                  a buffer (in ms) to lower the boundary for the default bound
    */
   protected static BackupBoundaries calculatePreservationBoundary(List<BackupInfo> backups,
-    long tsBuffer) {
+    Map<String, Set<TableName>> incrBackupTableSetPerRoot, long tsBuffer) {
     if (LOG.isDebugEnabled()) {
       LOG.debug(
         "Cleaning WALs if they are older than the WAL cleanup time-boundary. "
@@ -120,7 +124,16 @@ protected static BackupBoundaries calculatePreservationBoundary(List<BackupInfo>
     }
 
     BackupBoundaries.BackupBoundariesBuilder builder = BackupBoundaries.builder(tsBuffer);
-    newestBackupPerRootDir.values().forEach(builder::update);
+    for (Map.Entry<String, BackupInfo> rootEntry : newestBackupPerRootDir.entrySet()) {
+      String backupRoot = rootEntry.getKey();
+      BackupInfo backupInfo = rootEntry.getValue();
+      // Load the active backup table set for each backup root, so we can then skip
+      // tables that are no longer part of the backup set. This set accurately reflects
+      // which tables still have backups and may need future incremental backups.
+      Set<TableName> activeTables = incrBackupTableSetPerRoot.get(backupRoot);
+      builder.update(backupInfo, activeTables);
+    }
+
     BackupBoundaries boundaries = builder.build();
 
     if (LOG.isDebugEnabled()) {
@@ -151,7 +164,16 @@ public Iterable<FileStatus> getDeletableFiles(Iterable<FileStatus> files) {
       long tsBuffer = getConf().getLong(TS_BUFFER_KEY, TS_BUFFER_DEFAULT);
       List<BackupInfo> backupHistory = sysTable.getBackupHistory(
         i -> EnumSet.of(BackupState.COMPLETE, BackupState.RUNNING).contains(i.getState()));
-      boundaries = calculatePreservationBoundary(backupHistory, tsBuffer);
+      Map<String, Set<TableName>> incrBackupTableSetPerRoot = new HashMap<>();
+      for (BackupInfo backupInfo : backupHistory) {
+        String backupRootDir = backupInfo.getBackupRootDir();
+        if (!incrBackupTableSetPerRoot.containsKey(backupRootDir)) {
+          incrBackupTableSetPerRoot.put(backupRootDir,
+            sysTable.getIncrementalBackupTableSet(backupRootDir));
+        }
+      }
+      boundaries =
+        calculatePreservationBoundary(backupHistory, incrBackupTableSetPerRoot, tsBuffer);
     } catch (IOException ex) {
       LOG.error("Failed to analyse backup history with exception: {}. Retaining all logs",
         ex.getMessage(), ex);
@@ -206,4 +228,5 @@ private static boolean isHMasterWAL(Path path) {
       || fn.endsWith(MasterRegionFactory.ARCHIVED_WAL_SUFFIX)
       || path.toString().contains("/%s/".formatted(MasterRegionFactory.MASTER_STORE_DIR));
   }
+
 }
diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupBoundaries.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupBoundaries.java
@@ -20,6 +20,7 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Set;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.backup.BackupInfo;
@@ -126,12 +127,17 @@ private BackupBoundariesBuilder(long tsCleanupBuffer) {
      * @param backupInfo the most recent completed backup info for a backup root, or if there is no
      *                   such completed backup, the currently running backup.
      */
-    public void update(BackupInfo backupInfo) {
+    public void update(BackupInfo backupInfo, Set<TableName> activeTables) {
       switch (backupInfo.getState()) {
         case COMPLETE:
           // If a completed backup exists in the backup root, we want to protect all logs that
           // have been created since the log-roll that happened for that backup.
           for (TableName table : backupInfo.getTableSetTimestampMap().keySet()) {
+            if (!activeTables.contains(table)) {
+              LOG.debug("Skipping stale table {} in backup root {}: not in incremental backup set",
+                table, backupInfo.getBackupRootDir());
+              continue;
+            }
             for (Map.Entry<String, Long> entry : backupInfo.getTableSetTimestampMap().get(table)
               .entrySet()) {
               Address regionServerAddress = Address.fromString(entry.getKey());
diff --git a/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/master/TestBackupLogCleaner.java b/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/master/TestBackupLogCleaner.java
@@ -23,7 +23,10 @@
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
@@ -205,6 +208,154 @@ public void testBackupLogCleaner() throws Exception {
     }
   }
 
+  /**
+   * Verify that when a table is no longer in the backup set, it doesn't block WAL cleanup.
+   */
+  @Test
+  public void testRemovedBackupDoesNotPinWals() throws Exception {
+    Path backupRoot = new Path(BACKUP_ROOT_DIR, "staleRoot");
+
+    try {
+      BackupLogCleaner cleaner = new BackupLogCleaner();
+      cleaner.setConf(TEST_UTIL.getConfiguration());
+      Map<String, Object> params = new HashMap<>(1);
+      params.put(HMaster.MASTER, TEST_UTIL.getHBaseCluster().getMaster());
+      cleaner.init(params);
+
+      // Create FULL backup B1 with table1 and table2
+      String backupIdB1 =
+        backupTables(BackupType.FULL, Arrays.asList(table1, table2), backupRoot.toString());
+      assertTrue(checkSucceeded(backupIdB1));
+
+      Set<FileStatus> walFilesAfterB1 =
+        new LinkedHashSet<>(getListOfWALFiles(TEST_UTIL.getConfiguration()));
+
+      // Insert data so the next backup advances WAL positions for table1
+      Connection conn = TEST_UTIL.getConnection();
+      try (Table t1 = conn.getTable(table1)) {
+        for (int i = 0; i < NB_ROWS_IN_BATCH; i++) {
+          Put p = new Put(Bytes.toBytes("stale-row-t1" + i));
+          p.addColumn(famName, qualName, Bytes.toBytes("val" + i));
+          t1.put(p);
+        }
+      }
+
+      // Create FULL backup B2 with only table1.
+      // B2's tableSetTimestampMap carries forward the old timestamp from B1 for table2,
+      // while table1 gets a fresh timestamp: { table1: ts(B2), table2: ts(B1) }
+      String backupIdB2 =
+        backupTables(BackupType.FULL, Collections.singletonList(table1), backupRoot.toString());
+      assertTrue(checkSucceeded(backupIdB2));
+
+      Set<FileStatus> walFilesAfterB2 =
+        mergeAsSet(walFilesAfterB1, getListOfWALFiles(TEST_UTIL.getConfiguration()));
+
+      // Delete B1: since it is the only backup referencing table2, finalizeDelete will
+      // remove table2 from the incremental backup set for this root.
+      getBackupAdmin().deleteBackups(new String[] { backupIdB1 });
+
+      // table2 is no longer in the backup set, so the boundary = ts(B2) instead of
+      // min(ts(B2), ts(B1)) = ts(B1). WALs between B1 and B2 are now deletable.
+      Iterable<FileStatus> deletable = cleaner.getDeletableFiles(walFilesAfterB2);
+      assertTrue(toSet(deletable).containsAll(walFilesAfterB1),
+        "WALs after B1 should be deletable once stale tables are removed from incr set");
+    } finally {
+      TEST_UTIL.truncateTable(BackupSystemTable.getTableName(TEST_UTIL.getConfiguration())).close();
+    }
+  }
+
+  /**
+   * Similar as {@link #testRemovedBackupDoesNotPinWals()} but for the case where a table is
+   * removed, and hence no longer in any backup either.
+   */
+  @Test
+  public void testRemovedTableDoesNotPinWals() throws Exception {
+    Path backupRoot = new Path(BACKUP_ROOT_DIR, "removedTableRoot");
+
+    try {
+      BackupLogCleaner cleaner = new BackupLogCleaner();
+      cleaner.setConf(TEST_UTIL.getConfiguration());
+      cleaner.init(Map.of(HMaster.MASTER, TEST_UTIL.getHBaseCluster().getMaster()));
+
+      // F1: Full backup of table1 and table2
+      String backupIdF1 =
+        backupTables(BackupType.FULL, Arrays.asList(table1, table2), backupRoot.toString());
+      assertTrue(checkSucceeded(backupIdF1));
+
+      Set<FileStatus> walFilesAfterF1 =
+        new LinkedHashSet<>(getListOfWALFiles(TEST_UTIL.getConfiguration()));
+
+      // Insert data into both tables so the incremental has something to back up
+      Connection conn = TEST_UTIL.getConnection();
+      try (Table t1 = conn.getTable(table1)) {
+        for (int i = 0; i < NB_ROWS_IN_BATCH; i++) {
+          Put p = new Put(Bytes.toBytes("rem-t1-" + i));
+          p.addColumn(famName, qualName, Bytes.toBytes("val" + i));
+          t1.put(p);
+        }
+      }
+      try (Table t2 = conn.getTable(table2)) {
+        for (int i = 0; i < NB_ROWS_IN_BATCH; i++) {
+          Put p = new Put(Bytes.toBytes("rem-t2-" + i));
+          p.addColumn(famName, qualName, Bytes.toBytes("val" + i));
+          t2.put(p);
+        }
+      }
+
+      // I1: Incremental backup (for both table1 and table2)
+      String backupIdI1 =
+        backupTables(BackupType.INCREMENTAL, Arrays.asList(table1, table2), backupRoot.toString());
+      assertTrue(checkSucceeded(backupIdI1));
+
+      Set<FileStatus> walFilesAfterI1 =
+        mergeAsSet(walFilesAfterF1, getListOfWALFiles(TEST_UTIL.getConfiguration()));
+
+      // Delete table2 from HBase. After this point, no new backup can reference it.
+      TEST_UTIL.deleteTable(table2);
+
+      // F2: Full backup of table1 only
+      String backupIdF2 =
+        backupTables(BackupType.FULL, Collections.singletonList(table1), backupRoot.toString());
+      assertTrue(checkSucceeded(backupIdF2));
+
+      Set<FileStatus> walFilesAfterF2 =
+        mergeAsSet(walFilesAfterI1, getListOfWALFiles(TEST_UTIL.getConfiguration()));
+
+      // Insert more data into table1 so the incremental has something to back up
+      try (Table t1 = conn.getTable(table1)) {
+        for (int i = 0; i < NB_ROWS_IN_BATCH; i++) {
+          Put p = new Put(Bytes.toBytes("rem-t1b-" + i));
+          p.addColumn(famName, qualName, Bytes.toBytes("val" + i));
+          t1.put(p);
+        }
+      }
+
+      // I2: Incremental backup of table1 only
+      String backupIdI2 = backupTables(BackupType.INCREMENTAL, Collections.singletonList(table1),
+        backupRoot.toString());
+      assertTrue(checkSucceeded(backupIdI2));
+
+      Set<FileStatus> walFilesAfterI2 =
+        mergeAsSet(walFilesAfterF2, getListOfWALFiles(TEST_UTIL.getConfiguration()));
+
+      // Delete F1 and I1 (i.e. the backups that still referenced table2).
+      getBackupAdmin().deleteBackups(new String[] { backupIdF1, backupIdI1 });
+
+      // table2 no longer exists and F1/I1 (the only backups referencing it) are deleted.
+      // The boundary should now be based solely on the remaining backups (F2, I2) for table1.
+      // All WALs after I1 should be deletable.
+      Iterable<FileStatus> deletable = cleaner.getDeletableFiles(walFilesAfterI2);
+      assertTrue(toSet(deletable).containsAll(walFilesAfterI1),
+        "WALs after I1 should be deletable once removed table no longer pins them");
+    } finally {
+      // Recreate table2 if it was deleted during the test
+      if (!TEST_UTIL.getAdmin().tableExists(table2)) {
+        TEST_UTIL.createTable(table2, famName);
+      }
+      TEST_UTIL.truncateTable(BackupSystemTable.getTableName(TEST_UTIL.getConfiguration())).close();
+    }
+  }
+
   @Test
   public void testDoesNotDeleteWALsFromNewServers() throws Exception {
     Path backupRoot1 = new Path(BACKUP_ROOT_DIR, "backup1");
@@ -287,10 +438,11 @@ public void testDoesNotDeleteWALsFromNewServers() throws Exception {
   public void testCanDeleteFileWithNewServerWALs() {
     BackupInfo backup = new BackupInfo();
     backup.setState(BackupInfo.BackupState.COMPLETE);
-    backup.setTableSetTimestampMap(
-      Map.of(TableName.valueOf("table1"), Map.of("server1:60020", 1000000L)));
+    TableName table = TableName.valueOf("table1");
+    Map<String, Set<TableName>> incrTableSetPerRoot = Map.of("root1", Set.of(table));
+    backup.setTableSetTimestampMap(Map.of(table, Map.of("server1:60020", 1000000L)));
     BackupBoundaries boundaries =
-      BackupLogCleaner.calculatePreservationBoundary(List.of(backup), 0L);
+      BackupLogCleaner.calculatePreservationBoundary(List.of(backup), incrTableSetPerRoot, 0L);
 
     // Old WAL from before the backup
     Path oldWAL = new Path("/hbase/oldWALs/server1%2C60020%2C12345.500000");
@@ -315,14 +467,16 @@ public void testCanDeleteFileWithNewServerWALs() {
 
   @Test
   public void testFirstBackupProtectsFiles() {
+    TableName table = TableName.valueOf("table1");
+
     BackupInfo backup = new BackupInfo();
     backup.setBackupId("backup_1");
     backup.setState(BackupInfo.BackupState.RUNNING);
     backup.setStartTs(100L);
     // Running backups have no TableSetTimestampMap
 
     BackupBoundaries boundaries =
-      BackupLogCleaner.calculatePreservationBoundary(List.of(backup), 5L);
+      BackupLogCleaner.calculatePreservationBoundary(List.of(backup), null /* ignored */, 5L);
 
     // There's only a single backup, and it is still running, so it's a FULL backup.
     // We expect files preceding the snapshot are deletable, but files after the start are not.
@@ -340,10 +494,11 @@ public void testFirstBackupProtectsFiles() {
     backup2.setBackupId("backup_2");
     backup2.setState(BackupInfo.BackupState.COMPLETE);
     backup2.setStartTs(80L);
-    backup2
-      .setTableSetTimestampMap(Map.of(TableName.valueOf("table1"), Map.of("server1:60020", 90L)));
+    backup2.setTableSetTimestampMap(Map.of(table, Map.of("server1:60020", 90L)));
 
-    boundaries = BackupLogCleaner.calculatePreservationBoundary(List.of(backup, backup2), 5L);
+    Map<String, Set<TableName>> incrTableSetPerRoot = Map.of("root1", Set.of(table));
+    boundaries = BackupLogCleaner.calculatePreservationBoundary(List.of(backup, backup2),
+      incrTableSetPerRoot, 5L);
 
     path = new Path("/hbase/oldWALs/server1%2C60020%2C12345.89");
     assertTrue(BackupLogCleaner.canDeleteFile(boundaries, path));
