rmdmattingly commented on code in PR #6040:
URL: https://github.com/apache/hbase/pull/6040#discussion_r1750447301
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
@@ -80,25 +80,32 @@ public void init(Map<String, Object> params) {
}
}
- private Map<Address, Long> getServersToOldestBackupMapping(List<BackupInfo>
backups)
+ private Map<Address, Long>
serversToLatestUnusedLogTimestamp(List<BackupInfo> backups)
throws IOException {
- Map<Address, Long> serverAddressToLastBackupMap = new HashMap<>();
+ Map<Address, Long> serversToLatestUnusedLogTimestamp = new HashMap<>();
- Map<TableName, Long> tableNameBackupInfoMap = new HashMap<>();
- for (BackupInfo backupInfo : backups) {
+ Map<String, BackupInfo> mostRecentBackupPerRootDir = new HashMap<>();
+ for (BackupInfo backup : backups) {
+ BackupInfo existingEntry =
mostRecentBackupPerRootDir.get(backup.getBackupRootDir());
+ if (existingEntry == null || existingEntry.getStartTs() <
backup.getStartTs()) {
+ mostRecentBackupPerRootDir.put(backup.getBackupRootDir(), backup);
+ }
+ }
+
+ for (BackupInfo backupInfo : mostRecentBackupPerRootDir.values()) {
for (TableName table : backupInfo.getTables()) {
- tableNameBackupInfoMap.putIfAbsent(table, backupInfo.getStartTs());
- if (tableNameBackupInfoMap.get(table) <= backupInfo.getStartTs()) {
- tableNameBackupInfoMap.put(table, backupInfo.getStartTs());
- for (Map.Entry<String, Long> entry :
backupInfo.getTableSetTimestampMap().get(table)
- .entrySet()) {
-
serverAddressToLastBackupMap.put(Address.fromString(entry.getKey()),
entry.getValue());
+ for (Map.Entry<String, Long> entry :
backupInfo.getTableSetTimestampMap().get(table)
+ .entrySet()) {
+ Address address = Address.fromString(entry.getKey());
+ Long existingValue = serversToLatestUnusedLogTimestamp.get(address);
+ if (existingValue == null || existingValue > entry.getValue()) {
+ serversToLatestUnusedLogTimestamp.put(address, entry.getValue());
}
Review Comment:
If we want to store the latest, or largest, timestamp in the
`serversToLatestUnusedLogTimestamp` then I think we'd want to flip
`existingValue > entry.getValue()` to `existingValue < entry.getValue()` if
this conditional indicates that `entry.getValue()` should be added to the map
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
@@ -160,7 +167,7 @@ public boolean isStopped() {
return this.stopped;
}
- protected static boolean canDeleteFile(Map<Address, Long>
addressToLastBackupMap, Path path) {
+ protected static boolean canDeleteFile(Map<Address, Long>
addressToLatestUnusedLogTs, Path path) {
Review Comment:
Total nitpick, but we unified on "newest" in [that other
PR](https://github.com/apache/hbase/pull/6195), in case we'd like to use newest
here as well for the sake of consistency.
--
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]
