Re: [PR] HBASE-29261: Investigate flaw in backup deletion validation of PITR-critical backups and propose correct approach [hbase]

["vinayakphegde (via GitHub)" <gi...@apache.org>]

vinayakphegde commented on code in PR #6922:
URL: https://github.com/apache/hbase/pull/6922#discussion_r2074758030


##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupCommands.java:
##########
@@ -757,104 +759,127 @@ private List<TableName> getTablesDependentOnBackupForPITR(String backupId) throw
           return List.of();
         }
 
-        // Retrieve the tables with continuous backup enabled and their start times
+        // Retrieve the tables with continuous backup enabled along with their start times
         Map<TableName, Long> continuousBackupStartTimes =
           backupSystemTable.getContinuousBackupTableSet();
 
-        // Determine the PITR time window
+        // Calculate the PITR window by fetching configuration and current time
         long pitrWindowDays = getConf().getLong(CONF_CONTINUOUS_BACKUP_PITR_WINDOW_DAYS,
           DEFAULT_CONTINUOUS_BACKUP_PITR_WINDOW_DAYS);
         long currentTime = EnvironmentEdgeManager.getDelegate().currentTime();
-        final MutableLong pitrMaxStartTime =
-          new MutableLong(currentTime - TimeUnit.DAYS.toMillis(pitrWindowDays));
-
-        // For all tables, determine the earliest (minimum) continuous backup start time.
-        // This represents the actual earliest point-in-time recovery (PITR) timestamp
-        // that can be used, ensuring we do not go beyond the available backup data.
-        long minContinuousBackupStartTime = currentTime;
-        for (TableName table : targetBackup.getTableNames()) {
-          minContinuousBackupStartTime = Math.min(minContinuousBackupStartTime,
-            continuousBackupStartTimes.getOrDefault(table, currentTime));
-        }
-
-        // The PITR max start time should be the maximum of the calculated minimum continuous backup
-        // start time and the default PITR max start time (based on the configured window).
-        // This ensures that PITR does not extend beyond what is practically possible.
-        pitrMaxStartTime.set(Math.max(minContinuousBackupStartTime, pitrMaxStartTime.longValue()));
+        final long maxAllowedPITRTime = currentTime - TimeUnit.DAYS.toMillis(pitrWindowDays);
 
+        // Check each table associated with the target backup
         for (TableName table : targetBackup.getTableNames()) {
-          // This backup is not necessary for this table since it doesn't have PITR enabled
+          // Skip tables without continuous backup enabled
           if (!continuousBackupStartTimes.containsKey(table)) {
             continue;
           }
-          if (
-            !isValidPITRBackup(targetBackup, table, continuousBackupStartTimes,
-              pitrMaxStartTime.longValue())
-          ) {
-            continue; // This backup is not crucial for PITR of this table
+
+          // Calculate the PITR window this backup covers for the table
+          Optional<Pair<Long, Long>> coveredPitrWindow = getCoveredPitrWindowForTable(targetBackup,
+            continuousBackupStartTimes.get(table), maxAllowedPITRTime, currentTime);
+
+          // If this backup does not cover a valid PITR window for the table, skip
+          if (coveredPitrWindow.isEmpty()) {
+            continue;
           }
 
-          // Check if another valid full backup exists for this table
-          List<BackupInfo> backupHistory = backupSystemTable.getBackupInfos(BackupState.COMPLETE);
-          boolean hasAnotherValidBackup = backupHistory.stream()
-            .anyMatch(backup -> !backup.getBackupId().equals(backupId) && isValidPITRBackup(backup,
-              table, continuousBackupStartTimes, pitrMaxStartTime.longValue()));
+          System.out.println("Backup [" + targetBackup.getBackupId()
+            + "] covered PITR window for table [" + table + "]: " + coveredPitrWindow);

Review Comment:
   There's no logging currently enabled in that class, likely because these are user-facing commands where the intention is to directly output information rather than log it. Given that, I'll go ahead and skip adding a log statement here as well.



-- 
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: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org