From 0a5a7ff5c9f0a6ed7ed23cde4d5584f7e2e3e2c0 Mon Sep 17 00:00:00 2001 From: DieterDP <90392398+DieterDP-ng@users.noreply.github.com> Date: Fri, 17 May 2024 12:48:10 +0200 Subject: [PATCH] HBASE-28568 Incremental backup set does not correctly shrink (#5876) The incremental backup set is the set of tables included when an incremental backup is created, it is managed per backup root dir and contains all tables that are present in at least one backup (in that root dir). The incremental backup set can only shrink when backups are deleted. However, the implementation was incorrect, causing this set to never be able to shrink. Reviewed-by: Ray Mattingly Signed-off-by: Nick Dimiduk --- .../hbase/backup/impl/BackupAdminImpl.java | 40 ++++++++----------- .../hbase/backup/impl/BackupSystemTable.java | 31 +++++++------- .../hadoop/hbase/backup/TestBackupDelete.java | 25 ++++++++++++ 3 files changed, 58 insertions(+), 38 deletions(-) diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupAdminImpl.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupAdminImpl.java index f580fb0c47bb..e8c1b4d9caf9 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupAdminImpl.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupAdminImpl.java @@ -93,7 +93,6 @@ public BackupInfo getBackupInfo(String backupId) throws IOException { public int deleteBackups(String[] backupIds) throws IOException { int totalDeleted = 0; - Map> allTablesMap = new HashMap<>(); boolean deleteSessionStarted; boolean snapshotDone; @@ -129,20 +128,16 @@ public int deleteBackups(String[] backupIds) throws IOException { } snapshotDone = true; try { + List affectedBackupRootDirs = new ArrayList<>(); for (int i = 0; i < backupIds.length; i++) { BackupInfo info = sysTable.readBackupInfo(backupIds[i]); - if (info != null) { - String rootDir = info.getBackupRootDir(); - HashSet allTables = allTablesMap.get(rootDir); - if (allTables == null) { - allTables = new HashSet<>(); - allTablesMap.put(rootDir, allTables); - } - allTables.addAll(info.getTableNames()); - totalDeleted += deleteBackup(backupIds[i], sysTable); + if (info == null) { + continue; } + affectedBackupRootDirs.add(info.getBackupRootDir()); + totalDeleted += deleteBackup(backupIds[i], sysTable); } - finalizeDelete(allTablesMap, sysTable); + finalizeDelete(affectedBackupRootDirs, sysTable); // Finish sysTable.finishDeleteOperation(); // delete snapshot @@ -175,26 +170,23 @@ public int deleteBackups(String[] backupIds) throws IOException { /** * Updates incremental backup set for every backupRoot - * @param tablesMap map [backupRoot: {@code Set}] - * @param table backup system table + * @param backupRoots backupRoots for which to revise the incremental backup set + * @param table backup system table * @throws IOException if a table operation fails */ - private void finalizeDelete(Map> tablesMap, BackupSystemTable table) + private void finalizeDelete(List backupRoots, BackupSystemTable table) throws IOException { - for (String backupRoot : tablesMap.keySet()) { + for (String backupRoot : backupRoots) { Set incrTableSet = table.getIncrementalBackupTableSet(backupRoot); - Map> tableMap = + Map> tableMap = table.getBackupHistoryForTableSet(incrTableSet, backupRoot); - for (Map.Entry> entry : tableMap.entrySet()) { - if (entry.getValue() == null) { - // No more backups for a table - incrTableSet.remove(entry.getKey()); - } - } + + // Keep only the tables that are present in other backups + incrTableSet.retainAll(tableMap.keySet()); + + table.deleteIncrementalBackupTableSet(backupRoot); if (!incrTableSet.isEmpty()) { table.addIncrementalBackupTableSet(incrTableSet, backupRoot); - } else { // empty - table.deleteIncrementalBackupTableSet(backupRoot); } } } diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java index b64587b6fca6..bbcc3892ae71 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java @@ -86,8 +86,9 @@ *
    *
  • 1. Backup sessions rowkey= "session:"+backupId; value =serialized BackupInfo
    • *
  • 2. Backup start code rowkey = "startcode:"+backupRoot; value = startcode
    • - *
  • 3. Incremental backup set rowkey="incrbackupset:"+backupRoot; value=[list of tables]
    • - *
  • 4. Table-RS-timestamp map rowkey="trslm:"+backupRoot+table_name; value = map[RS-%3E last WAL + *
  • 3. Incremental backup set rowkey="incrbackupset:"+backupRoot; table="meta:"+tablename of + * include table; value=empty
    • + *
  • 4. Table-RS-timestamp map rowkey="trslm:"+backupRoot+table_name; value = map[RS-> last WAL * timestamp]
    • *
  • 5. RS - WAL ts map rowkey="rslogts:"+backupRoot +server; value = last WAL timestamp
    • *
  • 6. WALs recorded rowkey="wals:"+WAL unique file name; value = backupId and full WAL file @@ -839,23 +840,25 @@ public List getBackupHistoryForTable(TableName name) throws IOExcept return tableHistory; } - public Map> getBackupHistoryForTableSet(Set set, + /** + * Goes through all backup history corresponding to the provided root folder, and collects all + * backup info mentioning each of the provided tables. + * @param set the tables for which to collect the {@code BackupInfo} + * @param backupRoot backup destination path to retrieve backup history for + * @return a map containing (a subset of) the provided {@code TableName}s, mapped to a list of at + * least one {@code BackupInfo} + * @throws IOException if getting the backup history fails + */ + public Map> getBackupHistoryForTableSet(Set set, String backupRoot) throws IOException { List history = getBackupHistory(backupRoot); - Map> tableHistoryMap = new HashMap<>(); - for (Iterator iterator = history.iterator(); iterator.hasNext();) { - BackupInfo info = iterator.next(); - if (!backupRoot.equals(info.getBackupRootDir())) { - continue; - } + Map> tableHistoryMap = new HashMap<>(); + for (BackupInfo info : history) { List tables = info.getTableNames(); for (TableName tableName : tables) { if (set.contains(tableName)) { - ArrayList list = tableHistoryMap.get(tableName); - if (list == null) { - list = new ArrayList<>(); - tableHistoryMap.put(tableName, list); - } + List list = + tableHistoryMap.computeIfAbsent(tableName, k -> new ArrayList<>()); list.add(info); } } diff --git a/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupDelete.java b/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupDelete.java index 0c4d44d489d8..ef40bc63d086 100644 --- a/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupDelete.java +++ b/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupDelete.java @@ -17,6 +17,7 @@ */ package org.apache.hadoop.hbase.backup; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; import java.io.ByteArrayOutputStream; @@ -30,6 +31,7 @@ import org.apache.hadoop.hbase.testclassification.LargeTests; import org.apache.hadoop.hbase.util.EnvironmentEdge; import org.apache.hadoop.hbase.util.EnvironmentEdgeManager; +import org.apache.hadoop.thirdparty.com.google.common.collect.Sets; import org.apache.hadoop.util.ToolRunner; import org.junit.Assert; import org.junit.ClassRule; @@ -158,4 +160,27 @@ public long currentTime() { LOG.info(baos.toString()); assertTrue(output.indexOf("Deleted 1 backups") >= 0); } + + /** + * Verify that backup deletion updates the incremental-backup-set. + */ + @Test + public void testBackupDeleteUpdatesIncrementalBackupSet() throws Exception { + LOG.info("Test backup delete updates the incremental backup set"); + BackupSystemTable backupSystemTable = new BackupSystemTable(TEST_UTIL.getConnection()); + + String backupId1 = fullTableBackup(Lists.newArrayList(table1, table2)); + assertTrue(checkSucceeded(backupId1)); + assertEquals(Sets.newHashSet(table1, table2), + backupSystemTable.getIncrementalBackupTableSet(BACKUP_ROOT_DIR)); + + String backupId2 = fullTableBackup(Lists.newArrayList(table3)); + assertTrue(checkSucceeded(backupId2)); + assertEquals(Sets.newHashSet(table1, table2, table3), + backupSystemTable.getIncrementalBackupTableSet(BACKUP_ROOT_DIR)); + + getBackupAdmin().deleteBackups(new String[] { backupId1 }); + assertEquals(Sets.newHashSet(table3), + backupSystemTable.getIncrementalBackupTableSet(BACKUP_ROOT_DIR)); + } }