HBASE-29905 BackupLogCleaner: skip tables no longer in the backup set

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 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.

Author: janvanbesien-ngdata

hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java

@@ -18,14 +18,14 @@
 package org.apache.hadoop.hbase.backup.master;
 
 import static org.apache.hadoop.hbase.backup.BackupInfo.withState;
-
 import java.io.IOException;
 import java.time.Duration;
 import java.util.ArrayList;
 import java.util.Collections;
 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;
@@ -49,7 +49,6 @@
 import org.apache.yetus.audience.InterfaceAudience;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-
 import org.apache.hbase.thirdparty.org.apache.commons.collections4.IterableUtils;
 import org.apache.hbase.thirdparty.org.apache.commons.collections4.MapUtils;
 
@@ -119,13 +118,30 @@ private BackupBoundaries serverToPreservationBoundaryTs(BackupSystemTable sysTab
           .collect(Collectors.joining(", ")));
     }
 
+    // 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. The incrbackupset is populated
+    // on every full backup and pruned when backups are deleted, so it accurately reflects
+    // which tables still have backups and may need future incrementals.
+    Map<String, Set<TableName>> activeTablesPerRoot = new HashMap<>();
+    for (String backupRoot : newestBackupPerRootDir.keySet()) {
+      activeTablesPerRoot.put(backupRoot, sysTable.getIncrementalBackupTableSet(backupRoot));
+    }
+
     BackupBoundaries.BackupBoundariesBuilder builder =
       BackupBoundaries.builder(getConf().getLong(TS_BUFFER_KEY, TS_BUFFER_DEFAULT));
-    for (BackupInfo backupInfo : newestBackupPerRootDir.values()) {
+    for (Map.Entry<String, BackupInfo> rootEntry : newestBackupPerRootDir.entrySet()) {
+      String backupRoot = rootEntry.getKey();
+      BackupInfo backupInfo = rootEntry.getValue();
+      Set<TableName> activeTables = activeTablesPerRoot.get(backupRoot);
       long startCode = Long.parseLong(sysTable.readBackupStartCode(backupInfo.getBackupRootDir()));
       // Iterate over all tables in the timestamp map, which contains all tables covered in the
       // backup root, not just the tables included in that specific backup (which could be a subset)
       for (TableName table : backupInfo.getTableSetTimestampMap().keySet()) {
+        if (activeTables != null && !activeTables.contains(table)) {
+          LOG.debug("Skipping stale table {} in backup root {}: not in incremental backup set",
+            table, backupRoot);
+          continue;
+        }
         for (Map.Entry<String, Long> entry : backupInfo.getTableSetTimestampMap().get(table)
           .entrySet()) {
           builder.addBackupTimestamps(entry.getKey(), entry.getValue(), startCode);

hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/master/TestBackupLogCleaner.java

@@ -25,6 +25,7 @@
 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;
@@ -215,6 +216,59 @@ public void testBackupLogCleaner() throws Exception {
     }
   }
 
+  @Test
+  public void testRemovedTableDoesNotPinWals() 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("WALs after B1 should be deletable once stale tables are removed from incr set",
+        toSet(deletable).containsAll(walFilesAfterB1));
+    } finally {
+      TEST_UTIL.truncateTable(BackupSystemTable.getTableName(TEST_UTIL.getConfiguration())).close();
+    }
+  }
+
   @Test
   public void testDoesNotDeleteWALsFromNewServers() throws Exception {
     Path backupRoot1 = new Path(BACKUP_ROOT_DIR, "backup1");