-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-29846 Fix backup history ordering #7662
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<BackupInfo> getBackupHistory(BackupInfo.Filter... toInclude) throws IOException { | ||
| LOG.trace("get backup history from backup system table"); | ||
|
|
||
| List<BackupInfo> 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<TableName> getTablesIncludedInBackups() throws IOException { | ||
| // Incremental backups have the same tables as the preceding full backups | ||
| List<BackupInfo> 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<TableName, List<BackupInfo>> getBackupHistoryForTableSet(Set<TableNam | |
| } | ||
|
|
||
| /** | ||
| * Get all backup infos passing the given filters (ordered by ascending backup id) | ||
| * Get all backup information passing the given filters, ordered by descending backupId. I.e. from | ||
| * newest to oldest. | ||
| */ | ||
| public List<BackupInfo> getBackupInfos(BackupInfo.Filter... toInclude) throws IOException { | ||
| return getBackupInfos(Integer.MAX_VALUE, toInclude); | ||
| public List<BackupInfo> 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<BackupInfo> getBackupInfos(int n, BackupInfo.Filter... toInclude) throws IOException { | ||
| public List<BackupInfo> getBackupHistory(boolean newToOld, int n, BackupInfo.Filter... toInclude) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use an enum instead of a bool to represent the sort order. |
||
| 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<BackupInfo> combinedPredicate = Stream.of(toInclude) | ||
| .map(filter -> (Predicate<BackupInfo>) filter).reduce(Predicate::and).orElse(x -> true); | ||
|
|
||
| Scan scan = createScanForBackupHistory(); | ||
| Scan scan = createScanForBackupHistory(newToOld); | ||
| List<BackupInfo> 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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please replace this computation with Hmm and we should add reverse scan support to this method that we can avoid off-by-one errors in the future. |
||
| scan.withStartRow(startRow); | ||
| scan.withStopRow(stopRow); | ||
| if (newestFirst) { | ||
| scan.setReversed(true); | ||
| scan.withStartRow(stopRow); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: use |
||
| scan.withStopRow(startRow); | ||
| } else { | ||
| scan.withStartRow(startRow); | ||
| scan.withStopRow(stopRow); | ||
| } | ||
| scan.addFamily(BackupSystemTable.SESSIONS_FAMILY); | ||
| scan.readVersions(1); | ||
| return scan; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<BackupInfo> getHistory(Configuration conf, Path backupRootPath) | ||
| throws IOException { | ||
| // Get all (n) history from backup root destination | ||
|
|
||
| FileSystem fs = FileSystem.get(backupRootPath.toUri(), conf); | ||
| RemoteIterator<LocatedFileStatus> it = fs.listLocatedStatus(backupRootPath); | ||
| RemoteIterator<LocatedFileStatus> it; | ||
| try { | ||
| it = fs.listLocatedStatus(backupRootPath); | ||
| } catch (FileNotFoundException e) { | ||
| return Collections.emptyList(); | ||
| } | ||
|
|
||
| List<BackupInfo> infos = new ArrayList<>(); | ||
| while (it.hasNext()) { | ||
|
|
@@ -571,26 +580,15 @@ private static List<BackupInfo> getHistory(Configuration conf, Path backupRootPa | |
| } | ||
| } | ||
| // Sort | ||
| Collections.sort(infos, new Comparator<BackupInfo>() { | ||
| @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()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now a String sort, not a timestamp sort? Why not rely on the exiting |
||
| 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<BackupInfo> getHistory(Configuration conf, int n, Path backupRootPath, | ||
| BackupInfo.Filter... filters) throws IOException { | ||
| List<BackupInfo> infos = getHistory(conf, backupRootPath); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictly speaking, because the BackupInfos are stored using BackupIds as key, the ordering promised in this method will break over 261 years, when the epoch
9999999999becomes10000000000. This is a side effect of backupIds not being 0-padded when stored.I.e., the scan would return out-of-order results:
backup_10000000000backup_9999999998backup_9999999999But I guess that's acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems acceptable :)