-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-28696 Partition BackupSystemTable queries #6067
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java
Outdated
Show resolved
Hide resolved
cda4d70
to
01f1c9e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
01f1c9e
to
41ab60d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java
Outdated
Show resolved
Hide resolved
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, BufferedMutator
wraps up this use-case nicely. Please clean this up a bit more and it looks good for merge.
@@ -1885,6 +1887,13 @@ private String cellKeyToBackupSetName(Cell current) { | |||
return Bytes.toString(data).substring(SET_KEY_PREFIX.length()); | |||
} | |||
|
|||
private void executeBufferedMutations(Table table, List<? extends Mutation> mutations) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This helper function is now kinda weird. I think that you can replace all the above try (Table table = ...) { ... }
with try (BufferedMutator bm = ...) { ... }
. On quick glance, you never use the table
instance other than to get the name in order to create the BufferedMutator
instance.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one @rmdmattingly
Co-authored-by: Ray Mattingly <rmattingly@hubspot.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Co-authored-by: Ray Mattingly <rmattingly@hubspot.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Co-authored-by: Ray Mattingly <rmattingly@hubspot.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Co-authored-by: Ray Mattingly <rmattingly@hubspot.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Co-authored-by: Ray Mattingly <rmattingly@hubspot.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Co-authored-by: Ray Mattingly <rmattingly@hubspot.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
When successfully taking an incremental backup, one of our final steps is to delete bulk load metadata from the system table for the bulk loads that needed to be captured in the given backup. This means that we will basically truncate the entire bulk loads system table in a single batch of the deletes after successfully taking an incremental backup. Depending on your usage, one may run tons of bulk loads between backups, so this design is needlessly fragile. We should partition these deletes so that we never erroneously fail a backup due to this; there are a few other cases where we generated unbounded multi requests in the BackupSystemTable that this PR addresses too.
A few questions here:
@charlesconnell @ndimiduk