HBASE-29905 BackupLogCleaner: skip tables no longer in the backup set#7761
Open
janvanbesien wants to merge 1 commit intoapache:masterfrom
Open
HBASE-29905 BackupLogCleaner: skip tables no longer in the backup set#7761janvanbesien wants to merge 1 commit intoapache:masterfrom
janvanbesien wants to merge 1 commit intoapache:masterfrom
Conversation
janvanbesien
force-pushed
the
HBASE-29905_backup_cleaner_stale_entries
branch
from
5a9a2ef to
9f69699
Compare
Contributor
|
Hi Jan, 2 remarks regarding the format of the PR:
|
DieterDP-ng
reviewed
Feb 17, 2026
janvanbesien
force-pushed
the
HBASE-29905_backup_cleaner_stale_entries
branch
from
9f69699 to
81e9e0d
Compare
DieterDP-ng
force-pushed
the
HBASE-29905_backup_cleaner_stale_entries
branch
from
81e9e0d to
1f676bb
Compare
Contributor
|
I've force-pushed a rebase on the current master. |
DieterDP-ng
force-pushed
the
HBASE-29905_backup_cleaner_stale_entries
branch
from
1f676bb to
5e8a415
Compare
DieterDePaepe
requested changes
Apr 3, 2026
DieterDePaepe
left a comment
DieterDePaepe
left a comment
There was a problem hiding this comment.
A few minor changes to do, looks good overall.
There was a problem hiding this comment.
I rebased your changes to the latest master, and there was some refactoring involved. Would appreciate a second pair of eyes to take a look at the new code.
Author
There was a problem hiding this comment.
I fixed all your remarks. However, when trying to rebase on master, I noticed that the test seems to work now without any code changes. So either the problem has been fixed it in the mean time via another change, or my test is no good.
I pushed it like this (test only) but I should find some more time to deep dive.
There was a problem hiding this comment.
Two observations that I had while reviewing, but not something that needs fixing in this PR. Mainly thinking out loud, and might log issues for these.
- If a table is deleted, it will result in WAL files sticking around until all backups containing that deleted table are gone (because that is when the incremental backup set gets updated). If long backup retention times are used, this can still be problematic. There's room for further improvement here.
- If a table is deleted and recreated with the same name, the backup info will still have the timestamps of the deleted table, and WALs that are included in incremental backups will also contain data of the deleted table. This would lead to a corrupted restore.
|
@hgromer Could you also review this PR? |
…es in system:backup table BackupLogCleaner.serverToPreservationBoundaryTs() computes the WAL deletion boundary by iterating over tableSetTimestampMap from persisted BackupInfo sessions. This map can contain entries for tables that were once part of the backup set but have since been deleted (or had all their backups deleted). Their stale, old timestamps drag the minimum WAL boundary back, preventing old WALs from being cleaned up. Fix: when computing boundaries, load the incrbackupset per backup root from BackupSystemTable and skip tables not in the active set. The incrbackupset is populated on every full backup and pruned when backups are deleted, so it accurately reflects which tables still need WAL retention.
janvanbesien
force-pushed
the
HBASE-29905_backup_cleaner_stale_entries
branch
from
5e8a415 to
792959a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BackupLogCleaner.serverToPreservationBoundaryTs() computes the WAL deletion boundary by iterating over tableSetTimestampMap from persisted BackupInfo sessions. This map can contain entries for tables that were once part of the backup set but have since had all their backups deleted. Their stale, old timestamps drag the minimum WAL boundary back, preventing old WALs from being cleaned up.
Fix: when computing boundaries, load the incrbackupset per backup root from BackupSystemTable and skip tables not in the active set. The incrbackupset is populated on every full backup and pruned when backups are deleted, so it accurately reflects which tables still need WAL retention.