HBASE-26587 Introduce a new Admin API to change SFT implementation #4030
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
|
Still need to add more tests, at lease for AsyncAdmin. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
joshelser
requested changes
Jan 15, 2022
| @@ -2590,7 +2620,20 @@ String getOperationType() { | |||
|
|
|||
| @Override | |||
| String getOperationType() { | |||
| return "ENABLE"; | |||
| return "MODIFY"; | |||
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| private enum StoreFileTrackerState { | ||
| NEED_RESTORE, NEED_MIGRATION, NEED_FINISH_MIGRATION, ALREADY_FINISHED |
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.
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.
.../apache/hadoop/hbase/regionserver/storefiletracker/ModifyTableStoreFileTrackerProcedure.java
Show resolved
Hide resolved
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin3.java
Show resolved
Hide resolved
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. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Any other concerns? @joshelser |
|
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. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@joshelser Mind taking a look again? There is a 'change requested' so without your comment we can not go ahead... Thanks. |
wchevreuil
reviewed
Jan 24, 2022
hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java
Outdated
Show resolved
Hide resolved
hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java
Outdated
Show resolved
Hide resolved
| StoreFileTrackerState state = checkState(conf, dstSFT); | ||
| switch (state) { | ||
| case NEED_FINISH_PREVIOUS_MIGRATION_FIRST: | ||
| TableDescriptor td = createRestoreTableDescriptor(current, getRestoreSFT(conf)); |
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?
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.
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.
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?
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.
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?
There was a problem hiding this comment.
No, I think if no two MTPs can run concurrently, this is safe enough.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
wchevreuil
approved these changes
Jan 25, 2022
|
Hi, @joshelser ,do you still have any more concerns on this PR? Thanks. |
|
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
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.
No description provided.