ndimiduk commented on code in PR #7662:
URL: https://github.com/apache/hbase/pull/7662#discussion_r2781687586
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/util/BackupUtils.java:
##########
@@ -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());
Review Comment:
This is now a String sort, not a timestamp sort? Why not rely on the exiting
`Comparable<BackupInfo>` implementation -- `BackupInfo#compareTo()` already
does timestamp extraction and ordering. `infos.sort(Comparator.reversed())`
should do the trick.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java:
##########
@@ -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)
Review Comment:
Please use an enum instead of a bool to represent the sort order.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java:
##########
@@ -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)
Review Comment:
That seems acceptable :)
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java:
##########
@@ -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);
Review Comment:
nit: use `#withStartRow(stopRow, false)` to make the exclusivity required by
the reverse scan explicit.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java:
##########
@@ -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);
Review Comment:
Please replace this computation with `Scan#setStartStopRowForPrefixScan()`,
at least for the non-reversed scan.
Hmm and we should add reverse scan support to this method that we can avoid
off-by-one errors in the future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
