HBASE-29984 Support separate old WAL directories in backup - #8512
HBASE-29984 Support separate old WAL directories in backup#8512Sigma-Ma wants to merge 1 commit into
Conversation
| for (FileStatus oldlog : oldlogs) { | ||
| p = oldlog.getPath(); | ||
| currentLogFile = p.toString(); | ||
| List<String> oldlogs = BackupUtils.getFiles(fs, oldLogDir, new ArrayList<>(), path -> true); |
There was a problem hiding this comment.
can you double check if BackupUtils#parseHostFromOldLog would need to be changed when hbase.separate.oldlogdir.by.regionserver=true ? basically I checked the logic that when hbase.separate.oldlogdir.by.regionserver=true , there may have archived file without the host:port but the host name is part of the directory , e.g. oldWALs/10.0.0.1,60030,1700000000000/wal.00000000000000000001
There was a problem hiding this comment.
Good catch. I double-checked the built-in WAL creation and archive paths.
For the standard WAL providers, enabling hbase.separate.oldlogdir.by.regionserver only changes the archive directory. The WAL filename still normally contains the ServerName because WALFactory uses the ServerName as the WAL prefix and archiving preserves the original filename.
However, parseHostFromOldLog currently ignores the parent directory, so a path such as oldWALs//wal. would return null and the archived WAL would be skipped by incremental backup collection.
I agree that we should handle this problem. I will update parseHostFromOldLog to use a valid full ServerName from the parent directory, falling back to the existing filename parsing for the flat oldWAL layout, and adjust the regression test to cover a hostless WAL filename.
There was a problem hiding this comment.
BackupUtils.parseHostFromOldLog now checks whether the parent directory is a full ServerName and uses it to derive host:port. The existing filename parsing is retained as the fallback for the flat oldWAL layout.
I also updated TestIncrementalBackupManager to use oldWALs//wal., so the regression test now covers the hostless WAL filename case directly.
TestBackupUtils and TestIncrementalBackupManager both pass.
There was a problem hiding this comment.
Pull request overview
Updates incremental backup WAL discovery to handle the hbase.separate.oldlogdir.by.regionserver layout (archived WALs nested under per-RegionServer directories) by recursively collecting files from the old WAL directory, and adds a regression test to ensure those archived WALs are included in the incremental backup file list.
Changes:
- Switch
.oldlogs/old-WAL collection inIncrementalBackupManagerfromFileSystem.listStatus(direct children only) toBackupUtils.getFiles(recursive file listing). - Add
TestIncrementalBackupManagerto validate archived WALs inside a RegionServer-specific old WAL subdirectory are picked up for incremental backups.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java | Recursively enumerates archived WAL files under the old WAL directory so both flat and per-RS layouts work. |
| hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestIncrementalBackupManager.java | Adds a regression test that creates an archived WAL under a per-RS old WAL directory and asserts it is included in the incremental backup file list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
081314a to
aef153d
Compare
JIRA: https://issues.apache.org/jira/browse/HBASE-29984
What changes were proposed in this pull request?
This change updates
IncrementalBackupManagerto recursively collect archived WAL files under the old WAL directory instead of assuming that every direct child is a WAL file.It reuses
BackupUtils.getFilesso that both the existing flat layout and the per-RegionServer directory layout enabled byhbase.separate.oldlogdir.by.regionserverare supported.A regression test is added to verify that an archived WAL stored under a RegionServer-specific directory is included in the incremental backup file list.
BackupUtils.parseHostFromOldLognow derives the host and port from the parent directory when it is a fullServerName. It falls back to the existing WAL filename parsing for the flat old WAL layout.Why are the changes needed?
When
hbase.separate.oldlogdir.by.regionserveris enabled, archived WALs are stored in RegionServer-specific subdirectories under the old WAL directory.The existing backup code uses
FileSystem.listStatusand treats every direct child as a WAL file. It can therefore attempt to parse a directory as a WAL and fail during incremental backup log collection.Recursively collecting files supports both directory layouts without changing the existing WAL filtering behavior.
Without this handling, a path such as
oldWALs/<serverName>/wal.<timestamp>cannot be associated with its RegionServer because the WAL basename does not contain the host and port.IncrementalBackupManagerwould consequently skip the archived WAL.How was this patch tested?
Updated
TestIncrementalBackupManagerto create an archived WAL using the following layout:The test verifies that the host is obtained from the parent directory and that the archived WAL is included in the incremental backup file list.
Also ran
TestBackupUtilsto verify that the existing flat WAL filename parsing continues to work.