HBASE-29003 Proper bulk load tracking #6506
Merged
Conversation
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
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
DieterDP-ng
force-pushed
the
HBASE-29003
branch
from
46a5851 to
fbb7080
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
DieterDP-ng
force-pushed
the
HBASE-29003
branch
from
fbb7080 to
44307bf
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Member
|
Do you mind fixing this broken checkstyle while you're in there? |
DieterDP-ng
force-pushed
the
HBASE-29003
branch
2 times, most recently
from
15fdfe7 to
01b6b87
Compare
Contributor
Author
|
Fixed the checkstyle error. @ndimiduk @rmdmattingly the discussion on Jira about the best approach stopped. Should I wait for further feedback? Personally, I think the best solution would be to track in the backup system table whether a full backup is required or not. This tracking would have to occur through an Observer as is done in this ticket. Given that this PR solves a real issue, I'd suggest to include it, and leave the switch to the tracking system to a later commit. Note that users should update their config to include the new observer. Not sure if this counts as a migration since the backup is still considered experimental. Should be mentioned somewhere though, but unsure where. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Member
|
FYI @hgromer |
Member
This kind of change does demand a mention in the release notes. We have such a field on our Jira ticket schema, and it automatically gets populated at release time. |
Contributor
|
I had actually spun up a Jira to this effect, didn't realize there was already an open PR |
Contributor
Author
|
Any advice on how to progress this ticket? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
Test failure looks unrelated to this PR, flaky test? |
Contributor
|
Hey @DieterDP-ng, thanks for the work here and sorry that I've been slow to respond. I will take a look here today |
rmdmattingly
approved these changes
Mar 10, 2025
| Predicate<BulkLoad> filter) throws IOException { | ||
| try (Connection connection = ConnectionFactory.createConnection(config); | ||
| BackupSystemTable tbl = new BackupSystemTable(connection)) { | ||
| List<BulkLoad> bulkLoads = tbl.readBulkloadRows(List.of(tableName)); |
Contributor
There was a problem hiding this comment.
This is a concern for another time, but it's only a matter of time until the lack of pagination in BackupSystemTable#readBulkloadRows bites us
hgromer
reviewed
Mar 18, 2025
| backupManager.writeBackupStartCode(newStartCode); | ||
|
|
||
| backupManager | ||
| .deleteBulkLoadedRows(bulkLoadsToDelete.stream().map(BulkLoad::getRowKey).toList()); |
Contributor
There was a problem hiding this comment.
At my company, we have a similar patch applied to our fork, and we've run into issues with batch sizes that causes backup failures. This seems to happen when there are too many rows to delete, you end up with something like
Caused by: org.apache.hbase.thirdparty.com.google.protobuf.ServiceException: Rejecting large batch operation for current batch with firstRegionName: backup:system_bulk,,1739970553683.c3828af81a4b3847aa0f1612bf638713. , Requested Number of Rows: 2048 , Size Threshold: 1500
at org.apache.hadoop.hbase.regionserver.RSRpcServices.checkBatchSizeAndLogLargeSize(RSRpcServices.java:2721)
at org.apache.hadoop.hbase.regionserver.RSRpcServices.multi(RSRpcServices.java:2757)
at org.apache.hadoop.hbase.shaded.protobuf.generated.ClientProtos$ClientService$2.callBlockingMethod(ClientProtos.java:43520)
at org.apache.ha
It might be worth splitting up this call to delete if they are exceptionally large
Member
There was a problem hiding this comment.
@hgromer do you feel that splitting up the delete is required for this patch?
Contributor
There was a problem hiding this comment.
It might be worth splitting this up. Introducing this means we can get into a situation where backups are unable to succeed.
Contributor
Author
There was a problem hiding this comment.
No objection to splitting it. Updating the PR...
Contributor
Author
There was a problem hiding this comment.
Rather than doing batching here, I did it at BackupSystemTable.deleteBulkLoadedRows, since that method is used in multiple locations, and I guess they would all benefit from batching. See added commit (can be squashed).
Contributor
Author
There was a problem hiding this comment.
(For future reference: turned out batching was already built-in, see #6506 (comment))
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
hgromer
reviewed
Apr 2, 2025
| public void deleteBulkLoadedRows(List<byte[]> rows) throws IOException { | ||
| try (BufferedMutator bufferedMutator = connection.getBufferedMutator(bulkLoadTableName)) { | ||
| try (BufferedMutator bufferedMutator = connection | ||
| .getBufferedMutator(new BufferedMutatorParams(bulkLoadTableName).setMaxMutations(1000))) { |
Contributor
There was a problem hiding this comment.
Would it be possible to set this to the config value HConstants.BATCH_ROWS_THRESHOLD_NAME, seeing as that's the cluster limit for batches?
The configuration value must then be supplied by the operator launching the backup.
Contributor
Author
There was a problem hiding this comment.
This pointer led me to conclude that it's not needed to specify batching at all, because it's already present.
Our method uses Connection#getBufferedMutator, for which the only non-delegating implementation is ConnectionOverAsyncConnection#getBufferedMutator. That one calls AsyncConnectionImpl#getBufferedMutatorBuilder(TableName). In that method we can see it uses a AsyncConnectionConfiguration object that has already extracted a batching value:
AsyncConnectionConfiguration(Configuration conf) {
...
this.bufferedMutatorMaxMutations = conf.getInt(BUFFERED_MUTATOR_MAX_MUTATIONS_KEY,
conf.getInt(HConstants.BATCH_ROWS_THRESHOLD_NAME, BUFFERED_MUTATOR_MAX_MUTATIONS_DEFAULT));
}
So I'll remove my change that introduced the batchsize.
Contributor
There was a problem hiding this comment.
Ah good to know, so the only thing needed is for the operator to specify the configuration in the Connection object that is passed to the BackupAdminImp
👍
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
hgromer
approved these changes
Apr 8, 2025
Contributor
|
Retrying the build |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
The failed test runs OK for me. I guess another flaky test. |
DieterDP-ng
commented
Apr 14, 2025
| } | ||
| (coproc == null ? "" : coproc + ",") + observerClass); | ||
|
|
||
| String masterCoProc = conf.get(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY); |
Contributor
Author
There was a problem hiding this comment.
This probably belongs in the decorateMasterConfiguration method instead.
Contributor
There was a problem hiding this comment.
Do you want to make this change? If not, I'm happy to ship this PR
Contributor
Author
There was a problem hiding this comment.
Updated!
I also extracted all master-related stuff from the BackupObserver to BackupMasterObserver. Did this in the light of HBASE-29240, where there's more master-specific logic being added.
The HBase backup mechanism keeps track of which HFiles were bulk loaded, so they can be included in incremental backups. Before this ticket, these bulk load records were only deleted when an incremental backup is created. This commit adds 2 more locations: 1) after a full backup. Since a full backup already captures all data, this meant that unnecessary HFiles were being included in the next incremental backup. 2) after a table delete/truncate/CF-deletion. Previously, if an HFile was loaded before a table was cleared, the next incremental backup would effectively still include the HFile. This lead to incorrect data being restored. This commit also completely refactors & simplifies the test for this functionality.
DieterDP-ng
force-pushed
the
HBASE-29003
branch
from
864b9c7 to
9cf3008
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
rmdmattingly
approved these changes
Apr 21, 2025
rmdmattingly
pushed a commit
that referenced
this pull request
Apr 21, 2025
Signed-off-by: Ray Mattingly <[email protected]> The HBase backup mechanism keeps track of which HFiles were bulk loaded, so they can be included in incremental backups. Before this ticket, these bulk load records were only deleted when an incremental backup is created. This commit adds 2 more locations: 1) after a full backup. Since a full backup already captures all data, this meant that unnecessary HFiles were being included in the next incremental backup. 2) after a table delete/truncate/CF-deletion. Previously, if an HFile was loaded before a table was cleared, the next incremental backup would effectively still include the HFile. This lead to incorrect data being restored. This commit also completely refactors & simplifies the test for this functionality.
rmdmattingly
pushed a commit
that referenced
this pull request
Apr 21, 2025
Signed-off-by: Ray Mattingly <[email protected]> The HBase backup mechanism keeps track of which HFiles were bulk loaded, so they can be included in incremental backups. Before this ticket, these bulk load records were only deleted when an incremental backup is created. This commit adds 2 more locations: 1) after a full backup. Since a full backup already captures all data, this meant that unnecessary HFiles were being included in the next incremental backup. 2) after a table delete/truncate/CF-deletion. Previously, if an HFile was loaded before a table was cleared, the next incremental backup would effectively still include the HFile. This lead to incorrect data being restored. This commit also completely refactors & simplifies the test for this functionality.
rmdmattingly
pushed a commit
that referenced
this pull request
Apr 22, 2025
Signed-off-by: Ray Mattingly <[email protected]> The HBase backup mechanism keeps track of which HFiles were bulk loaded, so they can be included in incremental backups. Before this ticket, these bulk load records were only deleted when an incremental backup is created. This commit adds 2 more locations: 1) after a full backup. Since a full backup already captures all data, this meant that unnecessary HFiles were being included in the next incremental backup. 2) after a table delete/truncate/CF-deletion. Previously, if an HFile was loaded before a table was cleared, the next incremental backup would effectively still include the HFile. This lead to incorrect data being restored. This commit also completely refactors & simplifies the test for this functionality.
rmdmattingly
added a commit
that referenced
this pull request
Apr 22, 2025
Signed-off-by: Ray Mattingly <[email protected]> The HBase backup mechanism keeps track of which HFiles were bulk loaded, so they can be included in incremental backups. Before this ticket, these bulk load records were only deleted when an incremental backup is created. This commit adds 2 more locations: 1) after a full backup. Since a full backup already captures all data, this meant that unnecessary HFiles were being included in the next incremental backup. 2) after a table delete/truncate/CF-deletion. Previously, if an HFile was loaded before a table was cleared, the next incremental backup would effectively still include the HFile. This lead to incorrect data being restored. This commit also completely refactors & simplifies the test for this functionality. Co-authored-by: DieterDP <[email protected]>
rmdmattingly
pushed a commit
that referenced
this pull request
Apr 23, 2025
Signed-off-by: Ray Mattingly <[email protected]> The HBase backup mechanism keeps track of which HFiles were bulk loaded, so they can be included in incremental backups. Before this ticket, these bulk load records were only deleted when an incremental backup is created. This commit adds 2 more locations: 1) after a full backup. Since a full backup already captures all data, this meant that unnecessary HFiles were being included in the next incremental backup. 2) after a table delete/truncate/CF-deletion. Previously, if an HFile was loaded before a table was cleared, the next incremental backup would effectively still include the HFile. This lead to incorrect data being restored. This commit also completely refactors & simplifies the test for this functionality.
rmdmattingly
added a commit
that referenced
this pull request
Apr 23, 2025
Signed-off-by: Ray Mattingly <[email protected]> The HBase backup mechanism keeps track of which HFiles were bulk loaded, so they can be included in incremental backups. Before this ticket, these bulk load records were only deleted when an incremental backup is created. This commit adds 2 more locations: 1) after a full backup. Since a full backup already captures all data, this meant that unnecessary HFiles were being included in the next incremental backup. 2) after a table delete/truncate/CF-deletion. Previously, if an HFile was loaded before a table was cleared, the next incremental backup would effectively still include the HFile. This lead to incorrect data being restored. This commit also completely refactors & simplifies the test for this functionality. Co-authored-by: DieterDP <[email protected]>
rmdmattingly
added a commit
that referenced
this pull request
Apr 23, 2025
Signed-off-by: Ray Mattingly <[email protected]> The HBase backup mechanism keeps track of which HFiles were bulk loaded, so they can be included in incremental backups. Before this ticket, these bulk load records were only deleted when an incremental backup is created. This commit adds 2 more locations: 1) after a full backup. Since a full backup already captures all data, this meant that unnecessary HFiles were being included in the next incremental backup. 2) after a table delete/truncate/CF-deletion. Previously, if an HFile was loaded before a table was cleared, the next incremental backup would effectively still include the HFile. This lead to incorrect data being restored. This commit also completely refactors & simplifies the test for this functionality. Co-authored-by: DieterDP <[email protected]>
rmdmattingly
added a commit
that referenced
this pull request
Apr 25, 2025
Signed-off-by: Ray Mattingly <[email protected]> The HBase backup mechanism keeps track of which HFiles were bulk loaded, so they can be included in incremental backups. Before this ticket, these bulk load records were only deleted when an incremental backup is created. This commit adds 2 more locations: 1) after a full backup. Since a full backup already captures all data, this meant that unnecessary HFiles were being included in the next incremental backup. 2) after a table delete/truncate/CF-deletion. Previously, if an HFile was loaded before a table was cleared, the next incremental backup would effectively still include the HFile. This lead to incorrect data being restored. This commit also completely refactors & simplifies the test for this functionality. Co-authored-by: DieterDP <[email protected]>
rmdmattingly
added a commit
that referenced
this pull request
Apr 27, 2025
The HBase backup mechanism keeps track of which HFiles were bulk loaded, so they can be included in incremental backups. Before this ticket, these bulk load records were only deleted when an incremental backup is created. This commit adds 2 more locations: 1) after a full backup. Since a full backup already captures all data, this meant that unnecessary HFiles were being included in the next incremental backup. 2) after a table delete/truncate/CF-deletion. Previously, if an HFile was loaded before a table was cleared, the next incremental backup would effectively still include the HFile. This lead to incorrect data being restored. This commit also completely refactors & simplifies the test for this functionality. Co-authored-by: DieterDP <[email protected]>
mokai87
pushed a commit
to mokai87/hbase
that referenced
this pull request
Aug 7, 2025
Signed-off-by: Ray Mattingly <[email protected]> The HBase backup mechanism keeps track of which HFiles were bulk loaded, so they can be included in incremental backups. Before this ticket, these bulk load records were only deleted when an incremental backup is created. This commit adds 2 more locations: 1) after a full backup. Since a full backup already captures all data, this meant that unnecessary HFiles were being included in the next incremental backup. 2) after a table delete/truncate/CF-deletion. Previously, if an HFile was loaded before a table was cleared, the next incremental backup would effectively still include the HFile. This lead to incorrect data being restored. This commit also completely refactors & simplifies the test for this functionality. Co-authored-by: DieterDP <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
Note that this build on top of my older PR: #6134