Introduce a Snapshot Procedure #4115
Merged
Conversation
This file contains 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
|
🎊 +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. |
|
hi duo, I've made some progress recently.
If you have time, would you mind taking a look on this at your convenience and see if there is any problems ? Thanks. @Apache9 |
Apache9
reviewed
Feb 25, 2022
There was a problem hiding this comment.
Overall LGTM.
Only need to confirm that the fencing method is correct.
Thanks for the hard work @frostruan !
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SnapshotProcedure.java
Show resolved
Hide resolved
...e-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SnapshotRegionProcedure.java
Outdated
Show resolved
Hide resolved
...e-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SnapshotRegionProcedure.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @RestrictedApi(explanation = "Should only be called in tests", link = "", | ||
| allowedOnPath = ".*(/src/test/.*|TestSnapshotProcedure).java") |
There was a problem hiding this comment.
'src/test/.* ' contains TestSnapshotProcedure?
hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
Show resolved
Hide resolved
| .filter(p -> !p.isFinished()) | ||
| .filter(p -> p.getServerName() != null) | ||
| .forEach(p -> { | ||
| verifyWorkerAssigner.addUsedWorker(p.getServerName()); |
There was a problem hiding this comment.
What if the region server is already dead?
There was a problem hiding this comment.
I think it's harmless that we have dead servers in the WorkAssigner based on this consideration:
- If the remote server is dead, there will be a FailedRemoteDispatchException thrown when we dispatch remote procedures. The procedure-v2 framework is designed to be able to handle this. The upper layer procedure can notice that and reschedule a new procedure, or the procedure can retry itself.
- When acquire a new worker from WorkAssigner, we will always choose from the online server list. The dead server will not be able to become a candidate again.
- If there are too many dead servers in the WorkerAssigner, we can restart the master to clear it. This is a very lightweight operation.
There was a problem hiding this comment.
Seems this is the behavior of the SplitWALManager, so I guess it is OK.
|
|
||
| private void prepareSnapshot(MasterProcedureEnv env) | ||
| throws ProcedureSuspendedException, IOException { | ||
| if (isAnySplitOrMergeProcedureRunning(env)) { |
There was a problem hiding this comment.
So when arriving here, we can not schedule new split or merge procedures for this table any more as we have already record this procedure in the SnapshotManager?
There was a problem hiding this comment.
Yes. That's correct. In fact, in the current implementation, if a zk-coordinated snapshot is in progress, the split/merge procedure on the same table will be marked as FAILED.
|
Really thanks for your great suggestions and help, Duo. I will address it ASAP. @Apache9 |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Apache9
approved these changes
Mar 12, 2022
There was a problem hiding this comment.
Overall LGTM. Just need to fix some minor nits.
I think we can merge it to master and branch-2 first, and then polish other things.
Thanks @frostruan !
| .filter(p -> !p.isFinished()) | ||
| .filter(p -> p.getServerName() != null) | ||
| .forEach(p -> { | ||
| verifyWorkerAssigner.addUsedWorker(p.getServerName()); |
There was a problem hiding this comment.
Seems this is the behavior of the SplitWALManager, so I guess it is OK.
|
Really appreciate your help, Duo @Apache9 |
Apache9
pushed a commit
that referenced
this pull request
Mar 12, 2022
Signed-off-by: Duo Zhang <[email protected]>
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.