Uh oh!
There was an error while loading. Please reload this page.
HBASE-26587 Introduce a new Admin API to change SFT implementation - #4030
Merged
Conversation
Apache9
commented
Jan 13, 2022
ContributorAuthor
Still need to add more tests, at lease for AsyncAdmin. |
Apache-HBase
commented
Jan 13, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 13, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 13, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 14, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 14, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 14, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 14, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 14, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 14, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 14, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 14, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 14, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 14, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 14, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 14, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 15, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 15, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 15, 2022
🎊 +1 overall
This message was automatically generated. |
joshelser
requested changes
Jan 15, 2022
joshelser
left a comment
Member
There was a problem hiding this comment.
Other than a naming suggestion change, looks pretty good. Thinking out loud: this change doesn't require a change in hbase shell, so that's good.
| @Override | ||
| String getOperationType() { | ||
| return "ENABLE"; | ||
| return "MODIFY"; |
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| private enum StoreFileTrackerState { | ||
| NEED_RESTORE, NEED_MIGRATION, NEED_FINISH_MIGRATION, ALREADY_FINISHED |
Member
There was a problem hiding this comment.
NEED_RESTORE was a bit confusing to me and I had to read to understand what it meant. What about:
NEED_RESTORE->NEED_SET_MIGRATIONNEED_MIGRATION->NEED_START_MIGRATIONNEED_FINISH_MIGRATION(perfect)
If you like this, should also change the constant up in proto.
ContributorAuthor
There was a problem hiding this comment.
Finally I name them as
NEED_FINISH_PREVIOUS_MIGRATION_FIRSTNEED_START_MIGRATIONNEED_FINISH_MIGRATIONALREADY_FINISHED
And on the procedure state, I name them as
MODIFY_STORE_FILE_TRACKER_FINISH_PREVIOUS_MIGRATIONMODIFY_STORE_FILE_TRACKER_START_MIGRATIONMODIFY_STORE_FILE_TRACKER_FINISH_MIGRATION
Is this clear enough for you?
Thanks.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Apache9
commented
Jan 16, 2022
ContributorAuthor
Yes, we do not change the existing method so we do not need to change hbase shell right now. Filed HBASE-26673 for implementing the shell commands for this operation. |
Apache-HBase
commented
Jan 16, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 16, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 16, 2022
🎊 +1 overall
This message was automatically generated. |
Apache9
commented
Jan 18, 2022
ContributorAuthor
Any other concerns? @joshelser |
Apache9
commented
Jan 20, 2022
ContributorAuthor
Ping @joshelser . I think this is an important feature. After this we could introduce new shell command to migrate the all the tables for a cluster. Thanks. |
Apache-HBase
commented
Jan 20, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 20, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 20, 2022
🎊 +1 overall
This message was automatically generated. |
Apache9
commented
Jan 23, 2022
ContributorAuthor
@joshelser Mind taking a look again? There is a 'change requested' so without your comment we can not go ahead... Thanks. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| StoreFileTrackerState state = checkState(conf, dstSFT); | ||
| switch (state) { | ||
| case NEED_FINISH_PREVIOUS_MIGRATION_FIRST: | ||
| TableDescriptor td = createRestoreTableDescriptor(current, getRestoreSFT(conf)); |
Contributor
There was a problem hiding this comment.
I'm a bit confused here. Say we have three impls and the migration. Currently using impl1 and we submit migration to dst=impl2 (this triggers a MSFTP #1), followed by another migration to dst=impl3 (this triggers a MSFTP #2), then here we simply add a MTP to set impl2 tracker straight (no migration)? What if MSFTP #1 hasn't finished when this MTP starts run?
ContributorAuthor
There was a problem hiding this comment.
What do you mean by "if MSFTP #1 hasn't finished when this MTP starts run"? The parent procedure can only resume execution when the child procedures are done. So there will be no overlap.
And typically, we only need to schedule two MTP, which is the flow starts from NEED_START_MIGRATION. First MTP changes the SFT implementation to MIGRATION, and the second MTP changes the SFT implementation to the dst implementation.
And why we need NEED_FINISH_PREVIOUS_MIGRATION_FIRST, is because we need to consider the situation where the SFT implementation is already MIGRATION, then we need to finish the MIGRATION by scheduling a MTP first. Then we can go back to the above logic to do the migration. Of course if the dst implementation the current MIGRATION is exactly what we want, then finish it is enough for us. That's why we have the logic in preCheckAndTryRestoreSFT.
I could try to add more comments here to explain the logic.
ContributorAuthor
There was a problem hiding this comment.
Oh we already have comments in getState method. PTAL if it is clear enough for you @wchevreuil ? Thanks.
Contributor
There was a problem hiding this comment.
What do you mean by "if MSFTP #1 hasn't finished when this MTP starts run"? The parent procedure can only resume execution when the child procedures are done. So there will be no overlap.
What I meant is if two, subsequent, "change SFT" commands are triggered. Those would trigger separate "MSFTPs", with then independent child "MTPs". Or does a "MSFTP" hold a lock on the table, preventing this from happening?
ContributorAuthor
There was a problem hiding this comment.
We hold exclusive lock on the table, same as MTP. We will release the lock between steps but there is no difference between MTP. I can not recall how this is handled or we just do not care, could dig more later. But anyway, there is no difference with the current MTP implementation.
ContributorAuthor
There was a problem hiding this comment.
In TestModifyTableProcedure, there are several tests which run ModifyTableProcedure concurrently. I changed the log at the beginning of the executeFromState method of ModifyTableProcedure to debug, and find out that, although we scheduled the two MTP at almost the same time, the exeuction flow was always that, one MTP would wait for another to finish before executing...
Need to dig more. But anyway, I do not think this is a blocker issue for this PR?
Contributor
There was a problem hiding this comment.
No, I think if no two MTPs can run concurrently, this is safe enough.
Apache-HBase
commented
Jan 24, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 24, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 24, 2022
🎊 +1 overall
This message was automatically generated. |
wchevreuil
approved these changes
Jan 25, 2022
Apache9
commented
Jan 26, 2022
ContributorAuthor
Hi, @joshelser ,do you still have any more concerns on this PR? Thanks. |
Apache9
commented
Jan 28, 2022
ContributorAuthor
Anyway, I think we have already waited enough time... Let me merge the PR. |
Apache9 added a commit
to Apache9/hbase
that referenced
this pull request
Jan 29, 2022
…pache#4030) Signed-off-by: Wellington Ramos Chevreuil <wchevreuil@apache.org> Reviewed-by: Josh Elser <elserj@apache.org>
apurtell pushed a commit
to apurtell/hbase
that referenced
this pull request
Mar 18, 2022
…pache#4030) (apache#4080) Signed-off-by: Wellington Ramos Chevreuil <wchevreuil@apache.org> Reviewed-by: Josh Elser <elserj@apache.org>
apurtell pushed a commit
to apurtell/hbase
that referenced
this pull request
Mar 19, 2022
…pache#4030) (apache#4080) Signed-off-by: Wellington Ramos Chevreuil <wchevreuil@apache.org> Reviewed-by: Josh Elser <elserj@apache.org>
apurtell pushed a commit
to apurtell/hbase
that referenced
this pull request
Mar 25, 2022
…pache#4030) (apache#4080) Signed-off-by: Wellington Ramos Chevreuil <wchevreuil@apache.org> Reviewed-by: Josh Elser <elserj@apache.org>
apurtell pushed a commit
to apurtell/hbase
that referenced
this pull request
Mar 26, 2022
…pache#4030) (apache#4080) Signed-off-by: Wellington Ramos Chevreuil <wchevreuil@apache.org> Reviewed-by: Josh Elser <elserj@apache.org>
vinayakphegde pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
…pache#4030) (apache#4080) Signed-off-by: Wellington Ramos Chevreuil <wchevreuil@apache.org> Reviewed-by: Josh Elser <elserj@apache.org> Change-Id: I2668783c0903641ebef24d53d8e30018829735b8
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 freeto 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.
No description provided.