HBASE-29025: Enhance the full backup command to support Continuous Backup #6710
+713
−69
Conversation
This file contains 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
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
anmolnar
approved these changes
Feb 24, 2025
Comment on lines
+350
to
+354
| if (continuousBackup && !BackupType.FULL.equals(backupType)) { | ||
| System.out.println("ERROR: Continuous backup can Only be specified for Full Backup"); | ||
| printUsage(); | ||
| throw new IOException(INCORRECT_USAGE); | ||
| } |
There was a problem hiding this comment.
I'm just wondering, since continuous backup is meaningful only for full backups, what if we introduce a new backup type? Maybe we already discussed this, I can't remember.
Usage: hbase backup create <type> <backup_path> [options]
type "full" to create a full backup image
"incremental" to create an incremental backup image
"continuous" to create new continuous backup
backup_path Full path to store the backup image
There was a problem hiding this comment.
Yeah, we discussed this and agreed to go with a new flag for the full backup.
- A continuous backup just takes a full backup and starts the WAL replication process. This happens only once—after that, we only need to take full backups. So, I don’t see a need for a separate backup type.
- Also, introducing a new type would unnecessarily differentiate full and continuous backups.
- In the future, we want to phase out the old incremental backup implementation. The plan is to use WALs replicated through continuous backup for incremental backups. So eventually, we might just have FULL and INCREMENTAL backups, where incremental backups rely on continuously replicated WALs.
- From a code perspective, our backup code has a lot of(almost everywhere) if-else checks for backup types. Adding a new type would mean modifying all of them.
What do you think?
There was a problem hiding this comment.
Okay, I'm convinced let's go with this. I didn't remember the discussion.
Comment on lines
+152
to
+155
| if (backupInfo.isContinuousBackupEnabled()) { | ||
| handleContinuousBackup(admin); | ||
| } else { | ||
| handleNonContinuousBackup(admin); |
There was a problem hiding this comment.
If we introduced new backup type for continuous backup, you would be able to create a brand new class just for the continuous backup potententially sharing some code if needed.
This if-else branch is a code smell I believe.
There was a problem hiding this comment.
Otherwise code refactoring looks good to me.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR introduces a new parameter/option
-cb(--continuous-backup-enabled) for full backups. When specified, it enables continuous backup for the selected tables along with the full backup.Continuous backup leverages the HBase replication framework and uses
org.apache.hadoop.hbase.backup.replication.ContinuousBackupReplicationEndpoint(by default) to continuously stream WAL entries to the backup WAL directory defined byhbase.backup.continuous.wal.dir.