From 694d5f21d5162275f3a427f75ce330a24c57a944 Mon Sep 17 00:00:00 2001 From: Ray Mattingly Date: Tue, 3 Sep 2024 18:47:54 -0400 Subject: [PATCH 1/3] Improve BackupLogCleaner naming, debug logging --- .../hbase/backup/master/BackupLogCleaner.java | 44 +++++++++++++++---- 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java index 1b53aa1d67f9..52441dceddaa 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java @@ -23,6 +23,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.fs.Path; @@ -80,8 +81,13 @@ public void init(Map params) { } } - private Map getServersToOldestBackupMapping(List backups) - throws IOException { + private Map getServerToLastBackupTs(List backups) throws IOException { + if (LOG.isDebugEnabled()) { + LOG.debug( + "Cleaning WALs if they are older than the latest backups. Checking WALs against {} backups: {}", + backups.size(), + backups.stream().map(BackupInfo::getBackupId).sorted().collect(Collectors.joining(", "))); + } Map serverAddressToLastBackupMap = new HashMap<>(); Map tableNameBackupInfoMap = new HashMap<>(); @@ -98,6 +104,12 @@ private Map getServersToOldestBackupMapping(List back } } + if (LOG.isDebugEnabled()) { + for (Map.Entry entry : serverAddressToLastBackupMap.entrySet()) { + LOG.debug("Server: {}, Last Backup: {}", entry.getKey().getHostName(), entry.getValue()); + } + } + return serverAddressToLastBackupMap; } @@ -116,8 +128,7 @@ public Iterable getDeletableFiles(Iterable files) { Map addressToLastBackupMap; try { try (BackupManager backupManager = new BackupManager(conn, getConf())) { - addressToLastBackupMap = - getServersToOldestBackupMapping(backupManager.getBackupHistory(true)); + addressToLastBackupMap = getServerToLastBackupTs(backupManager.getBackupHistory(true)); } } catch (IOException ex) { LOG.error("Failed to analyse backup history with exception: {}. Retaining all logs", @@ -176,12 +187,29 @@ protected static boolean canDeleteFile(Map 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; + } + + if (LOG.isDebugEnabled()) { + LOG.debug( + "Backup found for server: {}. Backup from {} is older than the file, so keeping: {}", + walServerAddress.getHostName(), lastBackupTs, path); + } } catch (Exception ex) { LOG.warn("Error occurred while filtering file: {}. Ignoring cleanup of this log", path, ex); return false; From 63e5cde8b52cf3a6162c69e99812b940ed7bbb2f Mon Sep 17 00:00:00 2001 From: Ray Mattingly Date: Fri, 6 Sep 2024 09:23:04 -0400 Subject: [PATCH 2/3] feedback --- .../apache/hadoop/hbase/backup/master/BackupLogCleaner.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java index 52441dceddaa..816aa646bb01 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java @@ -84,7 +84,8 @@ public void init(Map params) { private Map getServerToLastBackupTs(List backups) throws IOException { if (LOG.isDebugEnabled()) { LOG.debug( - "Cleaning WALs if they are older than the latest backups. Checking WALs against {} backups: {}", + "Cleaning WALs if they are older than the latest backups. " + + "Checking WALs against {} backups: {}", backups.size(), backups.stream().map(BackupInfo::getBackupId).sorted().collect(Collectors.joining(", "))); } @@ -196,7 +197,7 @@ protected static boolean canDeleteFile(Map addressToLastBackupMap } Long lastBackupTs = addressToLastBackupMap.get(walServerAddress); - if (lastBackupTs > walTimestamp) { + if (lastBackupTs >= walTimestamp) { if (LOG.isDebugEnabled()) { LOG.debug( "Backup found for server: {}. Backup from {} is newer than file, so deleting: {}", From a756f534372cac422af6f17523e35e8ed1c6aa58 Mon Sep 17 00:00:00 2001 From: Ray Mattingly Date: Fri, 6 Sep 2024 10:51:27 -0400 Subject: [PATCH 3/3] clearer naming --- .../hbase/backup/master/BackupLogCleaner.java | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java index 816aa646bb01..1e09a6c4aba9 100644 --- a/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java +++ b/hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/master/BackupLogCleaner.java @@ -81,15 +81,16 @@ public void init(Map params) { } } - private Map getServerToLastBackupTs(List backups) throws IOException { + private Map getServerToNewestBackupTs(List backups) + throws IOException { if (LOG.isDebugEnabled()) { LOG.debug( - "Cleaning WALs if they are older than the latest backups. " + "Cleaning WALs if they are older than the newest backups. " + "Checking WALs against {} backups: {}", backups.size(), backups.stream().map(BackupInfo::getBackupId).sorted().collect(Collectors.joining(", "))); } - Map serverAddressToLastBackupMap = new HashMap<>(); + Map serverAddressToNewestBackupMap = new HashMap<>(); Map tableNameBackupInfoMap = new HashMap<>(); for (BackupInfo backupInfo : backups) { @@ -99,19 +100,20 @@ private Map getServerToLastBackupTs(List backups) thr tableNameBackupInfoMap.put(table, backupInfo.getStartTs()); for (Map.Entry entry : backupInfo.getTableSetTimestampMap().get(table) .entrySet()) { - serverAddressToLastBackupMap.put(Address.fromString(entry.getKey()), entry.getValue()); + serverAddressToNewestBackupMap.put(Address.fromString(entry.getKey()), + entry.getValue()); } } } } if (LOG.isDebugEnabled()) { - for (Map.Entry entry : serverAddressToLastBackupMap.entrySet()) { - LOG.debug("Server: {}, Last Backup: {}", entry.getKey().getHostName(), entry.getValue()); + for (Map.Entry entry : serverAddressToNewestBackupMap.entrySet()) { + LOG.debug("Server: {}, Newest Backup: {}", entry.getKey().getHostName(), entry.getValue()); } } - return serverAddressToLastBackupMap; + return serverAddressToNewestBackupMap; } @Override @@ -126,10 +128,10 @@ public Iterable getDeletableFiles(Iterable files) { return files; } - Map addressToLastBackupMap; + Map addressToNewestBackupMap; try { try (BackupManager backupManager = new BackupManager(conn, getConf())) { - addressToLastBackupMap = getServerToLastBackupTs(backupManager.getBackupHistory(true)); + addressToNewestBackupMap = getServerToNewestBackupTs(backupManager.getBackupHistory(true)); } } catch (IOException ex) { LOG.error("Failed to analyse backup history with exception: {}. Retaining all logs", @@ -137,7 +139,7 @@ public Iterable getDeletableFiles(Iterable files) { return Collections.emptyList(); } for (FileStatus file : files) { - if (canDeleteFile(addressToLastBackupMap, file.getPath())) { + if (canDeleteFile(addressToNewestBackupMap, file.getPath())) { filteredFiles.add(file); } } @@ -172,7 +174,7 @@ public boolean isStopped() { return this.stopped; } - protected static boolean canDeleteFile(Map addressToLastBackupMap, Path path) { + protected static boolean canDeleteFile(Map addressToNewestBackupMap, Path path) { if (isHMasterWAL(path)) { return true; } @@ -188,7 +190,7 @@ protected static boolean canDeleteFile(Map addressToLastBackupMap Address walServerAddress = Address.fromString(hostname); long walTimestamp = AbstractFSWALProvider.getTimestamp(path.getName()); - if (!addressToLastBackupMap.containsKey(walServerAddress)) { + if (!addressToNewestBackupMap.containsKey(walServerAddress)) { if (LOG.isDebugEnabled()) { LOG.debug("No backup found for server: {}. Deleting file: {}", walServerAddress.getHostName(), path); @@ -196,7 +198,7 @@ protected static boolean canDeleteFile(Map addressToLastBackupMap return true; } - Long lastBackupTs = addressToLastBackupMap.get(walServerAddress); + Long lastBackupTs = addressToNewestBackupMap.get(walServerAddress); if (lastBackupTs >= walTimestamp) { if (LOG.isDebugEnabled()) { LOG.debug(