rmdmattingly commented on code in PR #8694:
URL: https://github.com/apache/hbase/pull/8694#discussion_r4085338727
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java:
##########
@@ -248,6 +258,54 @@ private void performSnapshots(Admin admin) throws
IOException {
}
}
+ /**
+ * Scan all WAL files to find offline/decommissioned hosts and record their
max WAL timestamps in
+ * newTimestamps. This ensures subsequent incremental backups won't
re-include WALs already covered
+ * by this full backup's snapshot.
+ */
+ private void adjustTimestampsForOfflineHosts(Map<String, Long>
previousLogRollsByHost,
+ Map<String, Long> latestLogRollsByHost) throws IOException {
+ Path walRootDir = CommonFSUtils.getWALRootDir(conf);
+ Path logDir = new Path(walRootDir, HConstants.HREGION_LOGDIR_NAME);
+ Path oldLogDir = new Path(walRootDir, HConstants.HREGION_OLDLOGDIR_NAME);
+ FileSystem fs = walRootDir.getFileSystem(conf);
+
+ List<FileStatus> allLogs = new ArrayList<>();
+ for (FileStatus hostLogDir : fs.listStatus(logDir)) {
Review Comment:
`adjustTimestampsForOfflineHosts` lists `oldLogDir` non-recursively and then
treats every returned `FileStatus` as a WAL file. Archived WALs are not always
flat under `oldWALs`: `BackupUtils.parseHostFromOldLog` explicitly handles the
`oldWALs/<fullServerName>/<wal>` layout (`if (parent != null &&
ServerName.isFullServerName(parent.getName()))`), so per-server archive
subdirectories exist. For such a layout this code (a) never sees the actual
archived WAL files, so an offline host's real max WAL timestamp is never
recorded, and (b) feeds the *directory* into `BackupUtils.getCreationTime`,
which does `Long.parseLong(name.substring(lastIndexOf('.') + 1))` with no
validation — for a server-name directory like
`host.example.com,16020,1700000000000` that parses
`example.com,16020,1700000000000` and throws an unchecked
NumberFormatException. The same non-recursion means files under
`WALs/<server>-splitting` layouts are handled by the first loop only if the dir
name parses.
Fix: Walk `oldLogDir` recursively (e.g. `fs.listStatus` with a dir check, or
`CommonFSUtils`/`RemoteIterator` recursion) and skip any FileStatus where
`isDirectory()` is true rather than passing it to
`parseHostNameFromLogFile`/`getCreationTime`. Guard `getCreationTime` calls so
an unparsable name is logged and skipped instead of aborting the backup.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java:
##########
@@ -208,14 +215,17 @@ private void handleContinuousBackup(Admin admin) throws
IOException {
}
private void handleNonContinuousBackup(Admin admin) throws IOException {
+ Map<String, Long> previousLogRollsByHost =
backupManager.readRegionServerLastLogRollResult();
performLogRoll();
+ Map<String, Long> latestLogRollsByHost = newTimestamps;
performBackupSnapshots(admin);
backupManager.addIncrementalBackupTableSet(backupInfo.getTables());
// set overall backup status: complete. Here we make sure to complete the
backup.
// After this checkpoint, even if entering cancel process, will let the
backup finished
backupInfo.setState(BackupState.COMPLETE);
+ adjustTimestampsForOfflineHosts(previousLogRollsByHost,
latestLogRollsByHost);
Review Comment:
The new WAL-directory scan is invoked after
`backupInfo.setState(BackupState.COMPLETE)` — past the point the surrounding
comment describes as the checkpoint after which "even if entering cancel
process, will let the backup finished". Any IOException from the scan
(`fs.listStatus(oldLogDir)` throws FileNotFoundException when `oldWALs` has not
been created yet, e.g. a freshly initialized WAL root) or an unchecked parse
failure now propagates out of `handleNonContinuousBackup` before
`updateBackupMetadata()` runs, leaving a completed snapshot with no persisted
timestamp map.
Fix: Either move the scan before the COMPLETE checkpoint, or wrap it so a
scan failure logs a warning and falls back to the log-roll-derived
`newTimestamps` instead of failing the backup after the snapshot has been
taken. Also guard against a non-existent `oldLogDir` with an `fs.exists` check.
--
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]
