ndimiduk commented on code in PR #6195:
URL: https://github.com/apache/hbase/pull/6195#discussion_r1743309099
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
@@ -80,8 +81,13 @@ public void init(Map<String, Object> params) {
}
}
- private Map<Address, Long> getServersToOldestBackupMapping(List<BackupInfo>
backups)
- throws IOException {
+ private Map<Address, Long> getServerToLastBackupTs(List<BackupInfo> backups)
throws IOException {
Review Comment:
imho this method name is still confusing. What's the relationship between
"last" and "oldest"? Can backups not be chronologically ordered? How about
`getServerToOldestBackupTS`, and the log message can talk about cleaning WALs
that are older than the **oldest** backups.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
@@ -176,12 +187,29 @@ protected static boolean canDeleteFile(Map<Address, Long>
addressToLastBackupMap
Address walServerAddress = Address.fromString(hostname);
long walTimestamp = AbstractFSWALProvider.getTimestamp(path.getName());
- if (
- !addressToLastBackupMap.containsKey(walServerAddress)
- || addressToLastBackupMap.get(walServerAddress) >= walTimestamp
- ) {
+ if (!addressToLastBackupMap.containsKey(walServerAddress)) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("No backup found for server: {}. Deleting file: {}",
+ walServerAddress.getHostName(), path);
+ }
return true;
}
+
+ Long lastBackupTs = addressToLastBackupMap.get(walServerAddress);
+ if (lastBackupTs > walTimestamp) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "Backup found for server: {}. Backup from {} is newer than file,
so deleting: {}",
+ walServerAddress.getHostName(), lastBackupTs, path);
+ }
+ return true;
Review Comment:
This is not a debugging refactor -- this new if-block introduces new
cleaning logic, yes?
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java:
##########
@@ -176,12 +187,29 @@ protected static boolean canDeleteFile(Map<Address, Long>
addressToLastBackupMap
Address walServerAddress = Address.fromString(hostname);
long walTimestamp = AbstractFSWALProvider.getTimestamp(path.getName());
- if (
- !addressToLastBackupMap.containsKey(walServerAddress)
- || addressToLastBackupMap.get(walServerAddress) >= walTimestamp
- ) {
+ if (!addressToLastBackupMap.containsKey(walServerAddress)) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("No backup found for server: {}. Deleting file: {}",
+ walServerAddress.getHostName(), path);
+ }
return true;
}
+
+ Long lastBackupTs = addressToLastBackupMap.get(walServerAddress);
+ if (lastBackupTs > walTimestamp) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "Backup found for server: {}. Backup from {} is newer than file,
so deleting: {}",
Review Comment:
You could make operators' lives easier by including human readable
timestamps in these log lines that are intended for humans. I think the whole
backup system would benefit from this.
--
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]
