From c0bb414b7fb3582efc385b6eb4f2890d7b7dc653 Mon Sep 17 00:00:00 2001 From: Dieter De Paepe Date: Thu, 22 Jan 2026 11:29:35 +0100 Subject: [PATCH] HBASE-29846 Fix backup history ordering Restores the ordering of BackupAdmin#getHistory that was accidentally reversed in HBASE-29808. Extends & refactors TestBackupShowHistory to verify correct behavior. Fixes a possible FileNotFoundException in BackupUtils#getHistory. Merges BackupSystemTable#getBackupHistory with BackupSystemTable#getBackupInfos, to further simplify backup info retrieval. Optimized various usages of history retrieval. Clarified some javadoc regarding backup history retrieval. --- .../hadoop/hbase/backup/BackupAdmin.java | 8 +- .../hbase/backup/impl/BackupAdminImpl.java | 7 +- .../hbase/backup/impl/BackupCommands.java | 8 +- .../hbase/backup/impl/BackupManager.java | 3 +- .../hbase/backup/impl/BackupSystemTable.java | 51 +++--- .../hadoop/hbase/backup/util/BackupUtils.java | 34 ++-- .../hbase/backup/TestBackupShowHistory.java | 154 +++++++++--------- 7 files changed, 135 insertions(+), 130 deletions(-) diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupAdmin.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupAdmin.java index a44c8843c993..fc8e79011cdb 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupAdmin.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/BackupAdmin.java @@ -77,10 +77,10 @@ public interface BackupAdmin extends Closeable { void mergeBackups(String[] backupIds) throws IOException; /** - * Show backup history command with filters - * @param n last n backup sessions - * @param f list of filters - * @return list of backup info objects + * Retrieve info about the most recent backups. + * @param n number of backup infos desired + * @param f optional filters, only entries passing the filters will be returned + * @return a list of at most n entries, ordered from newest (most recent) to oldest (least recent) * @throws IOException exception */ List getHistory(int n, BackupInfo.Filter... f) throws IOException; 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 0044918b077c..4b2a76272907 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 @@ -78,7 +78,8 @@ public BackupInfo getBackupInfo(String backupId) throws IOException { BackupInfo backupInfo; try (final BackupSystemTable table = new BackupSystemTable(conn)) { if (backupId == null) { - List recentSessions = table.getBackupInfos(withState(BackupState.RUNNING)); + List recentSessions = + table.getBackupHistory(true, 1, withState(BackupState.RUNNING)); if (recentSessions.isEmpty()) { LOG.warn("No ongoing sessions found."); return null; @@ -115,7 +116,7 @@ public int deleteBackups(String[] backupIds) throws IOException { } // Step 2: Make sure there is no failed session - List list = sysTable.getBackupInfos(withState(BackupState.RUNNING)); + List list = sysTable.getBackupHistory(true, 1, withState(BackupState.RUNNING)); if (list.size() != 0) { // ailed sessions found LOG.warn("Failed backup session found. Run backup repair tool first."); @@ -374,7 +375,7 @@ private boolean isLastBackupSession(BackupSystemTable table, TableName tn, long @Override public List getHistory(int n, BackupInfo.Filter... filters) throws IOException { try (final BackupSystemTable table = new BackupSystemTable(conn)) { - return table.getBackupInfos(n, filters); + return table.getBackupHistory(true, n, filters); } } diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupCommands.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupCommands.java index 2926b7a8ee63..0a3b1549a641 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupCommands.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupCommands.java @@ -154,7 +154,8 @@ public void execute() throws IOException { if (requiresNoActiveSession()) { // Check active session try (BackupSystemTable table = new BackupSystemTable(conn)) { - List sessions = table.getBackupInfos(withState(BackupState.RUNNING)); + List sessions = + table.getBackupHistory(true, 1, withState(BackupState.RUNNING)); if (sessions.size() > 0) { System.err.println("Found backup session in a RUNNING state: "); @@ -529,7 +530,8 @@ public void execute() throws IOException { if (backupId != null) { info = sysTable.readBackupInfo(backupId); } else { - List infos = sysTable.getBackupInfos(withState(BackupState.RUNNING)); + List infos = + sysTable.getBackupHistory(true, 1, withState(BackupState.RUNNING)); if (infos != null && infos.size() > 0) { info = infos.get(0); backupId = info.getBackupId(); @@ -677,7 +679,7 @@ public void execute() throws IOException { final BackupSystemTable sysTable = new BackupSystemTable(conn)) { // Failed backup BackupInfo backupInfo; - List list = sysTable.getBackupInfos(withState(BackupState.RUNNING)); + List list = sysTable.getBackupHistory(true, 1, withState(BackupState.RUNNING)); if (list.size() == 0) { // No failed sessions found System.out.println("REPAIR status: no failed sessions found." diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java index 9cab455441bb..79c39e211a2d 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManager.java @@ -251,7 +251,8 @@ public BackupInfo createBackupInfo(String backupId, BackupType type, List sessions = systemTable.getBackupInfos(withState(BackupState.RUNNING)); + List sessions = + systemTable.getBackupHistory(true, 1, withState(BackupState.RUNNING)); if (sessions.size() == 0) { return null; } 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 3b46335a7299..8487d842e5f2 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 @@ -30,7 +30,6 @@ import java.util.Arrays; import java.util.Collection; import java.util.Collections; -import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; @@ -82,6 +81,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.apache.hbase.thirdparty.com.google.common.base.Preconditions; import org.apache.hbase.thirdparty.com.google.common.base.Splitter; import org.apache.hbase.thirdparty.com.google.common.collect.Iterators; @@ -597,25 +597,13 @@ public void writeRegionServerLastLogRollResult(String server, Long ts, String ba } } - /** - * Get all backup information passing the given filters, ordered by descending start time. I.e. - * from newest to oldest. - */ - public List getBackupHistory(BackupInfo.Filter... toInclude) throws IOException { - LOG.trace("get backup history from backup system table"); - - List list = getBackupInfos(toInclude); - list.sort(Comparator.comparing(BackupInfo::getStartTs).reversed()); - return list; - } - /** * Retrieve all table names that are part of any known completed backup */ public Set getTablesIncludedInBackups() throws IOException { // Incremental backups have the same tables as the preceding full backups List infos = - getBackupInfos(withState(BackupState.COMPLETE), withType(BackupType.FULL)); + getBackupHistory(withState(BackupState.COMPLETE), withType(BackupType.FULL)); return infos.stream().flatMap(info -> info.getTableNames().stream()) .collect(Collectors.toSet()); } @@ -647,26 +635,32 @@ public Map> getBackupHistoryForTableSet(Set getBackupInfos(BackupInfo.Filter... toInclude) throws IOException { - return getBackupInfos(Integer.MAX_VALUE, toInclude); + public List getBackupHistory(BackupInfo.Filter... toInclude) throws IOException { + return getBackupHistory(true, Integer.MAX_VALUE, toInclude); } /** - * Get the first n backup infos passing the given filters (ordered by ascending backup id) + * Retrieves the first n entries of the sorted, filtered list of backup infos. + * @param newToOld sort by descending backupId if true (i.e. newest to oldest), or by ascending + * backupId (i.e. oldest to newest) if false + * @param n number of entries to return */ - public List getBackupInfos(int n, BackupInfo.Filter... toInclude) throws IOException { + public List getBackupHistory(boolean newToOld, int n, BackupInfo.Filter... toInclude) + throws IOException { + Preconditions.checkArgument(n >= 0, "n should be >= 0"); LOG.trace("get backup infos from backup system table"); - if (n <= 0) { + if (n == 0) { return Collections.emptyList(); } Predicate combinedPredicate = Stream.of(toInclude) .map(filter -> (Predicate) filter).reduce(Predicate::and).orElse(x -> true); - Scan scan = createScanForBackupHistory(); + Scan scan = createScanForBackupHistory(newToOld); List list = new ArrayList<>(); try (Table table = connection.getTable(tableName); @@ -859,7 +853,7 @@ public boolean hasBackupSessions() throws IOException { LOG.trace("Has backup sessions from backup system table"); boolean result = false; - Scan scan = createScanForBackupHistory(); + Scan scan = createScanForBackupHistory(false); scan.setCaching(1); try (Table table = connection.getTable(tableName); ResultScanner scanner = table.getScanner(scan)) { @@ -1190,15 +1184,22 @@ private Delete createDeleteForIncrBackupTableSet(String backupRoot) { /** * Creates Scan operation to load backup history + * @param newestFirst if true, result are ordered by descending backupId * @return scan operation */ - private Scan createScanForBackupHistory() { + private Scan createScanForBackupHistory(boolean newestFirst) { Scan scan = new Scan(); byte[] startRow = Bytes.toBytes(BACKUP_INFO_PREFIX); byte[] stopRow = Arrays.copyOf(startRow, startRow.length); stopRow[stopRow.length - 1] = (byte) (stopRow[stopRow.length - 1] + 1); - scan.withStartRow(startRow); - scan.withStopRow(stopRow); + if (newestFirst) { + scan.setReversed(true); + scan.withStartRow(stopRow); + scan.withStopRow(startRow); + } else { + scan.withStartRow(startRow); + scan.withStopRow(stopRow); + } scan.addFamily(BackupSystemTable.SESSIONS_FAMILY); scan.readVersions(1); return scan; diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupUtils.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupUtils.java index 033975c42484..9eb0b319a718 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupUtils.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupUtils.java @@ -547,12 +547,21 @@ public static String getLogBackupDir(String backupRootDir, String backupId) { + HConstants.HREGION_LOGDIR_NAME; } + /** + * Loads all backup history as stored in files on the given backup root path. + * @return all backup history, from newest (most recent) to oldest (least recent) + */ private static List getHistory(Configuration conf, Path backupRootPath) throws IOException { // Get all (n) history from backup root destination FileSystem fs = FileSystem.get(backupRootPath.toUri(), conf); - RemoteIterator it = fs.listLocatedStatus(backupRootPath); + RemoteIterator it; + try { + it = fs.listLocatedStatus(backupRootPath); + } catch (FileNotFoundException e) { + return Collections.emptyList(); + } List infos = new ArrayList<>(); while (it.hasNext()) { @@ -571,26 +580,15 @@ private static List getHistory(Configuration conf, Path backupRootPa } } // Sort - Collections.sort(infos, new Comparator() { - @Override - public int compare(BackupInfo o1, BackupInfo o2) { - long ts1 = getTimestamp(o1.getBackupId()); - long ts2 = getTimestamp(o2.getBackupId()); - - if (ts1 == ts2) { - return 0; - } - - return ts1 < ts2 ? 1 : -1; - } - - private long getTimestamp(String backupId) { - return Long.parseLong(Iterators.get(Splitter.on('_').split(backupId).iterator(), 1)); - } - }); + infos.sort(Comparator.comparing(BackupInfo::getBackupId).reversed()); return infos; } + /** + * Loads all backup history as stored in files on the given backup root path, and returns the + * first n entries matching all given filters. + * @return (subset of) backup history, from newest (most recent) to oldest (least recent) + */ public static List getHistory(Configuration conf, int n, Path backupRootPath, BackupInfo.Filter... filters) throws IOException { List infos = getHistory(conf, backupRootPath); diff --git a/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupShowHistory.java b/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupShowHistory.java index 165003fdb5cb..a3d5470a7e64 100644 --- a/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupShowHistory.java +++ b/hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupShowHistory.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; @@ -24,7 +25,6 @@ import java.util.List; import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.HBaseClassTestRule; -import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.backup.util.BackupUtils; import org.apache.hadoop.hbase.testclassification.LargeTests; import org.apache.hadoop.util.ToolRunner; @@ -34,8 +34,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import org.apache.hbase.thirdparty.com.google.common.collect.Lists; - @Category(LargeTests.class) public class TestBackupShowHistory extends TestBackupBase { @@ -45,96 +43,100 @@ public class TestBackupShowHistory extends TestBackupBase { private static final Logger LOG = LoggerFactory.getLogger(TestBackupShowHistory.class); - private boolean findBackup(List history, String backupId) { - assertTrue(history.size() > 0); - boolean success = false; - for (BackupInfo info : history) { - if (info.getBackupId().equals(backupId)) { - success = true; - break; - } - } - return success; - } - /** - * Verify that full backup is created on a single table with data correctly. Verify that history - * works as expected. - * @throws Exception if doing the backup or an operation on the tables fails + * Verify that backup history retrieval works as expected. */ @Test public void testBackupHistory() throws Exception { - LOG.info("test backup history on a single table with data"); - List tableList = Lists.newArrayList(table1); - String backupId = fullTableBackup(tableList); + // Test without backups present + List history = getBackupAdmin().getHistory(10); + assertEquals(0, history.size()); + + history = BackupUtils.getHistory(conf1, 10, new Path(BACKUP_ROOT_DIR)); + assertEquals(0, history.size()); + + // Create first backup + String backupId = fullTableBackup(List.of(table1)); assertTrue(checkSucceeded(backupId)); LOG.info("backup complete"); - List history = getBackupAdmin().getHistory(10); - assertTrue(findBackup(history, backupId)); + // Tests with one backup present + history = getBackupAdmin().getHistory(10); + assertEquals(1, history.size()); + assertEquals(backupId, history.get(0).getBackupId()); + history = BackupUtils.getHistory(conf1, 10, new Path(BACKUP_ROOT_DIR)); - assertTrue(findBackup(history, backupId)); + assertEquals(1, history.size()); + assertEquals(backupId, history.get(0).getBackupId()); + String output = runHistoryCommand(10); + assertTrue(output.indexOf(backupId) > 0); + + // Create second backup + String backupId2 = fullTableBackup(List.of(table2)); + assertTrue(checkSucceeded(backupId2)); + LOG.info("backup complete: " + table2); + + // Test with multiple backups + history = getBackupAdmin().getHistory(10); + assertEquals(2, history.size()); + assertEquals(backupId2, history.get(0).getBackupId()); + assertEquals(backupId, history.get(1).getBackupId()); + + history = BackupUtils.getHistory(conf1, 10, new Path(BACKUP_ROOT_DIR)); + assertEquals(2, history.size()); + assertEquals(backupId2, history.get(0).getBackupId()); + assertEquals(backupId, history.get(1).getBackupId()); + + output = runHistoryCommand(10); + int idx1 = output.indexOf(backupId); + int idx2 = output.indexOf(backupId2); + assertTrue(idx1 >= 0); // Backup 1 is listed + assertTrue(idx2 >= 0); // Backup 2 is listed + assertTrue(idx2 < idx1); // Newest backup (Backup 2) comes first + + // Test with multiple backups & n == 1 + history = getBackupAdmin().getHistory(1); + assertEquals(1, history.size()); + assertEquals(backupId2, history.get(0).getBackupId()); + + history = BackupUtils.getHistory(conf1, 1, new Path(BACKUP_ROOT_DIR)); + assertEquals(1, history.size()); + assertEquals(backupId2, history.get(0).getBackupId()); + + output = runHistoryCommand(1); + idx1 = output.indexOf(backupId); + idx2 = output.indexOf(backupId2); + assertTrue(idx2 > 0); // most recent backup is listed + assertEquals(-1, idx1); // second most recent backup isn't listed + + // Test with multiple backups & filtering + BackupInfo.Filter tableNameFilter = i -> i.getTableNames().contains(table1); + + history = getBackupAdmin().getHistory(10, tableNameFilter); + assertEquals(1, history.size()); + assertEquals(backupId, history.get(0).getBackupId()); + + history = BackupUtils.getHistory(conf1, 10, new Path(BACKUP_ROOT_DIR), tableNameFilter); + assertEquals(1, history.size()); + assertEquals(backupId, history.get(0).getBackupId()); + + } + + private String runHistoryCommand(int n) throws Exception { + String[] args = new String[] { "history", "-n", String.valueOf(n), "-p", BACKUP_ROOT_DIR }; ByteArrayOutputStream baos = new ByteArrayOutputStream(); System.setOut(new PrintStream(baos)); - String[] args = new String[] { "history", "-n", "10", "-p", BACKUP_ROOT_DIR }; - // Run backup + LOG.info("Running history command"); int ret = ToolRunner.run(conf1, new BackupDriver(), args); - assertTrue(ret == 0); - LOG.info("show_history"); + assertEquals(0, ret); + String output = baos.toString(); LOG.info(output); baos.close(); - assertTrue(output.indexOf(backupId) > 0); - - tableList = Lists.newArrayList(table2); - String backupId2 = fullTableBackup(tableList); - assertTrue(checkSucceeded(backupId2)); - LOG.info("backup complete: " + table2); - BackupInfo.Filter tableNameFilter = image -> { - if (table1 == null) { - return true; - } - - List names = image.getTableNames(); - return names.contains(table1); - }; - BackupInfo.Filter tableSetFilter = info -> { - String backupId1 = info.getBackupId(); - return backupId1.startsWith("backup"); - }; - - history = getBackupAdmin().getHistory(10, tableNameFilter, tableSetFilter); - assertTrue(history.size() > 0); - boolean success = true; - for (BackupInfo info : history) { - if (!info.getTableNames().contains(table1)) { - success = false; - break; - } - } - assertTrue(success); - - history = - BackupUtils.getHistory(conf1, 10, new Path(BACKUP_ROOT_DIR), tableNameFilter, tableSetFilter); - assertTrue(history.size() > 0); - success = true; - for (BackupInfo info : history) { - if (!info.getTableNames().contains(table1)) { - success = false; - break; - } - } - assertTrue(success); - - args = - new String[] { "history", "-n", "10", "-p", BACKUP_ROOT_DIR, "-t", "table1", "-s", "backup" }; - // Run backup - ret = ToolRunner.run(conf1, new BackupDriver(), args); - assertTrue(ret == 0); - LOG.info("show_history"); + return output; } }