From 23a36d8f74c45c61ef9dd5c877e6e7b41edc5cd9 Mon Sep 17 00:00:00 2001 From: Dieter De Paepe Date: Mon, 6 May 2024 16:42:56 +0200 Subject: [PATCH 1/4] HBASE-28568 Fix incremental backup set shrinking 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. --- .../hbase/backup/impl/BackupAdminImpl.java | 35 +++++++------------ .../hbase/backup/impl/BackupSystemTable.java | 26 +++++++------- 2 files changed, 27 insertions(+), 34 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..731cef75998d 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,15 @@ 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()); + affectedBackupRootDirs.add(info.getBackupRootDir()); totalDeleted += deleteBackup(backupIds[i], sysTable); } } - finalizeDelete(allTablesMap, sysTable); + finalizeDelete(affectedBackupRootDirs, sysTable); // Finish sysTable.finishDeleteOperation(); // delete snapshot @@ -175,26 +169,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 682757dbc404..c8f2c51104f9 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 @@ -839,23 +839,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); } } From 704474d8a81ae0474c733c141bbce945bad2ef2f Mon Sep 17 00:00:00 2001 From: Dieter De Paepe Date: Wed, 8 May 2024 15:03:58 +0200 Subject: [PATCH 2/4] Add test & javadoc fix --- .../hbase/backup/impl/BackupSystemTable.java | 3 ++- .../hadoop/hbase/backup/TestBackupDelete.java | 25 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) 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 c8f2c51104f9..c364316d54eb 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,7 +86,8 @@ *
    *
  • 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]
    • + *
  • 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
    • 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..dbd914f6f445 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(backupId1)); + 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)); + } } From a41d516c7b9e51a173045232413d73940fb99138 Mon Sep 17 00:00:00 2001 From: Dieter De Paepe Date: Wed, 8 May 2024 16:52:30 +0200 Subject: [PATCH 3/4] Fix typo --- .../java/org/apache/hadoop/hbase/backup/TestBackupDelete.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 dbd914f6f445..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 @@ -175,7 +175,7 @@ public void testBackupDeleteUpdatesIncrementalBackupSet() throws Exception { backupSystemTable.getIncrementalBackupTableSet(BACKUP_ROOT_DIR)); String backupId2 = fullTableBackup(Lists.newArrayList(table3)); - assertTrue(checkSucceeded(backupId1)); + assertTrue(checkSucceeded(backupId2)); assertEquals(Sets.newHashSet(table1, table2, table3), backupSystemTable.getIncrementalBackupTableSet(BACKUP_ROOT_DIR)); From e705aeaa8e5f8a7ba7b67c0ce9572cfb25b504b6 Mon Sep 17 00:00:00 2001 From: Dieter De Paepe Date: Mon, 13 May 2024 18:41:50 +0200 Subject: [PATCH 4/4] Address nit --- .../apache/hadoop/hbase/backup/impl/BackupAdminImpl.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 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 731cef75998d..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 @@ -131,10 +131,11 @@ public int deleteBackups(String[] backupIds) throws IOException { List affectedBackupRootDirs = new ArrayList<>(); for (int i = 0; i < backupIds.length; i++) { BackupInfo info = sysTable.readBackupInfo(backupIds[i]); - if (info != null) { - affectedBackupRootDirs.add(info.getBackupRootDir()); - totalDeleted += deleteBackup(backupIds[i], sysTable); + if (info == null) { + continue; } + affectedBackupRootDirs.add(info.getBackupRootDir()); + totalDeleted += deleteBackup(backupIds[i], sysTable); } finalizeDelete(affectedBackupRootDirs, sysTable); // Finish