-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-28990 Modify Incremental Backup for Continuous Backup #6788
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2104620
fcee6f7
430676c
ba54bad
dc809b3
8e1d033
7b26cb0
39a4f8c
da0703f
bbc34b6
940f6c0
945774c
eb6f9f4
64fd7b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -859,28 +859,47 @@ public String backupTables(BackupRequest request) throws IOException { | |
|
|
||
| String backupId = BackupRestoreConstants.BACKUPID_PREFIX + EnvironmentEdgeManager.currentTime(); | ||
| if (type == BackupType.INCREMENTAL) { | ||
| Set<TableName> incrTableSet; | ||
| try (BackupSystemTable table = new BackupSystemTable(conn)) { | ||
| incrTableSet = table.getIncrementalBackupTableSet(targetRootDir); | ||
| } | ||
| if (request.isContinuousBackupEnabled()) { | ||
| Set<TableName> continuousBackupTableSet; | ||
| try (BackupSystemTable table = new BackupSystemTable(conn)) { | ||
| continuousBackupTableSet = table.getContinuousBackupTableSet().keySet(); | ||
| } | ||
| if (continuousBackupTableSet.isEmpty()) { | ||
| String msg = "Continuous backup table set contains no tables. " | ||
| + "You need to run Continuous backup first " | ||
| + (tableList != null ? "on " + StringUtils.join(tableList, ",") : ""); | ||
| throw new IOException(msg); | ||
| } | ||
| if (!continuousBackupTableSet.containsAll(tableList)) { | ||
| String extraTables = StringUtils.join(tableList, ","); | ||
| String msg = "Some tables (" + extraTables + ") haven't gone through Continuous backup. " | ||
| + "Perform Continuous backup on " + extraTables + " first, then retry the command"; | ||
| throw new IOException(msg); | ||
| } | ||
| } else { | ||
| Set<TableName> incrTableSet; | ||
| try (BackupSystemTable table = new BackupSystemTable(conn)) { | ||
| incrTableSet = table.getIncrementalBackupTableSet(targetRootDir); | ||
| } | ||
|
|
||
| if (incrTableSet.isEmpty()) { | ||
| String msg = | ||
| "Incremental backup table set contains no tables. " + "You need to run full backup first " | ||
| if (incrTableSet.isEmpty()) { | ||
| String msg = "Incremental backup table set contains no tables. " | ||
| + "You need to run full backup first " | ||
| + (tableList != null ? "on " + StringUtils.join(tableList, ",") : ""); | ||
|
|
||
| throw new IOException(msg); | ||
| } | ||
| if (tableList != null) { | ||
| tableList.removeAll(incrTableSet); | ||
| if (!tableList.isEmpty()) { | ||
| String extraTables = StringUtils.join(tableList, ","); | ||
| String msg = "Some tables (" + extraTables + ") haven't gone through full backup. " | ||
| + "Perform full backup on " + extraTables + " first, " + "then retry the command"; | ||
| throw new IOException(msg); | ||
| } | ||
| if (tableList != null) { | ||
| tableList.removeAll(incrTableSet); | ||
| if (!tableList.isEmpty()) { | ||
| String extraTables = StringUtils.join(tableList, ","); | ||
| String msg = "Some tables (" + extraTables + ") haven't gone through full backup. " | ||
| + "Perform full backup on " + extraTables + " first, then retry the command"; | ||
| throw new IOException(msg); | ||
| } | ||
| } | ||
| tableList = Lists.newArrayList(incrTableSet); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @rmdmattingly, I had a quick question for clarification — this is not related to the changes in this PR, but rather about the current incremental backup process. It seems like we're using incrTableSet to determine the tables for incremental backup. Could you help us understand why we consider all tables marked for incremental backup instead of just the user-specified tables? Understanding the reasoning behind this will help us better integrate the continuous backup functionality with the existing logic. Appreciate your insights — thank you!
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reason: [HBASE-14038] The incremental backup is controlled by the 'incremental backup table set'. |
||
| } | ||
| tableList = Lists.newArrayList(incrTableSet); | ||
| } | ||
| if (tableList != null && !tableList.isEmpty()) { | ||
| for (TableName table : tableList) { | ||
|
|
@@ -907,7 +926,12 @@ public String backupTables(BackupRequest request) throws IOException { | |
| } | ||
| } | ||
| if (nonExistingTableList != null) { | ||
| if (type == BackupType.INCREMENTAL) { | ||
| // Non-continuous incremental backup is controlled by 'incremental backup table set' | ||
| // and not by user provided backup table list. This is an optimization to avoid copying | ||
| // the same set of WALs for incremental backups of different tables at different times | ||
| // HBASE-14038. Since continuous incremental backup and full backup backs-up user provided | ||
| // table list, we should inform use about non-existence of input table(s) | ||
| if (type == BackupType.INCREMENTAL && !request.isContinuousBackupEnabled()) { | ||
ankitsol marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // Update incremental backup set | ||
| tableList = excludeNonExistingTables(tableList, nonExistingTableList); | ||
| } else { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -149,12 +149,20 @@ public void execute() throws IOException { | |
| try (Admin admin = conn.getAdmin()) { | ||
| beginBackup(backupManager, backupInfo); | ||
|
|
||
| // Gather the bulk loads being tracked by the system, which can be deleted (since their data | ||
| // will be part of the snapshot being taken). We gather this list before taking the actual | ||
| // snapshots for the same reason as the log rolls. | ||
| List<BulkLoad> bulkLoadsToDelete = backupManager.readBulkloadRows(tableList); | ||
ankitsol marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I couldn’t quite figure out why this step is needed. Could you please explain the reasoning behind it?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is needed so subsequent incremental backup (just after this full backup) captures only bulkload from this full backup, so fullbackup deletes existing bulkload entries from system table So an incremental backup post a full backup will capture only bulkload entries from fullbackup This change is actually part of https://github.com/apache/hbase/pull/6506/files#diff-18c753bdf4ce717d55d98646bf46723970d2bd4a470a815eb9512c7d94398274 which is merged, I added here because it was necessary for bulkload test I added
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I have a few questions:
@anmolnar @kgeisz @taklwu — please share your thoughts as well.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For 1) & 2) points I see 2 advantages with the coded approach in this PR. First, this behaviour would be same for both continuous and non-continuous incremental backup (ie using bulkload files from source cluster). Second, using source cluster hfiles wrt bulkload operation instead of backup hfiles would reduce processing time during backup and cost of storage space of backup area. Backing up bulkload operation would also delay backup of WALs since backing up WALs and bulkload files are serial in execution. Backing bulkload files idea was necessary when we were planning to use WALPlayer with bulkload restore capability. Now I don't see any advantage of backing up of bulkload files
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the question is what happens if the source cluster/storage is gone and users need a DR?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Kota-SH Post bulkload operation, user is advised to take a backup, as part of continuous backup and PITR design (I think this is not yet documented, I will add a comment in design to add this part) Earlier we implemented WALPlayer with bulkload restore functionality but that could have resulted in timeouts or performance issues, so we dropped this WALPlayer modification and decided to ask user to take a backup (ideally incremental backup) post a bulkload operation so restore is fast
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Kota-SH Regarding compaction, there is preCommitStoreFile() hook which registers these bulkloaded files in backup system table and avoids deletion during compaction
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so, maybe asking differently, is this a one-way approach for continuous backup if we couple with the optimization of HBASE-29003 that reduce the additional bulkload HFiles of the source cluster? and without this change, or as @Kota-SH pointed about the if the source cluster/directory is not accessible, which backup can we use ? especially can we still have incremental recovery? My two cents on this approach, building on top of HBASE-29003, is that it seems reasonable. at least incremental backup already has this code change that uses the source cluster/storage. I was wondered the feedback on the design docs is also suggesting us to work closer with the logic of incremental backup, and such we could avoid introducing similar logic but in fact is serving the same thing. Meanwhile, it's worth thinking of point 3 and 4, I assumed @ankitsol already answered, so I don't have comments.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense — thanks for the discussion, folks! 👍 |
||
|
|
||
| if (backupInfo.isContinuousBackupEnabled()) { | ||
| handleContinuousBackup(admin); | ||
| } else { | ||
| handleNonContinuousBackup(admin); | ||
| } | ||
|
|
||
| backupManager | ||
| .deleteBulkLoadedRows(bulkLoadsToDelete.stream().map(BulkLoad::getRowKey).toList()); | ||
|
|
||
| completeBackup(conn, backupInfo, BackupType.FULL, conf); | ||
| } catch (Exception e) { | ||
| failBackup(conn, backupInfo, backupManager, e, "Unexpected BackupException : ", | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.