HBASE-28366 Mis-order of SCP and regionServerReport results into region inconsistencies #5774
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
…on inconsistencies
|
🎊 +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. |
Apache9
reviewed
Mar 25, 2024
| @@ -324,8 +324,19 @@ public void regionServerReport(ServerName sn, ServerMetrics sl) throws YouAreDea | |||
| // the ServerName to use. Here we presume a master has already done | |||
| // that so we'll press on with whatever it gave us for ServerName. | |||
| if (!checkAndRecordNewServer(sn, sl)) { | |||
| LOG.info("RegionServerReport ignored, could not record the server: " + sn); | |||
| return; // Not recorded, so no need to move on | |||
| // Master already registered server with same (host + port) and higher startcode. | |||
There was a problem hiding this comment.
I still do not think this is necessary, because if the new server with the same host and port has already registered to master, how can we return this YouAreDeadException to the old server? Even if there is a race condition, when sending we will receive a connection reset because the old server is already dead...
There was a problem hiding this comment.
When it happened (as per logs mentioned on the jira), master processed the report and that generated inconsistencies.
We have seen this happen many times in the past when regionserver is not really aborted but looses connection with Zookeeper, triggering SCP by master. And regionserver with new startcode is not only alive but has also reported regionservers to master. After that, somehow master still receives regionserver report from old startcode regionserver, master processes it and that results into inconsistencies. I know this is rare case but it definitely happened more than once in more than one prod clusters.
There was a problem hiding this comment.
This is more of safety check, it will prevent inconsistencies. I agree that anyone looking at this would think, why do we need such extra safety, it's valid point but I can guarantee that not having such strict validation has caused inconsistencies.
There was a problem hiding this comment.
The other way to think about this is: why should we even receive any report from old server and not throw YouAreDeadException while we already know that new server is alive and is already registered? 🙂
There was a problem hiding this comment.
I checked the code, now I understand why we need to throw an exception here. Your comment totally missed the most important part...
At least your comment should include these two points:
- The exception thrown here is not meant to tell the region server it is dead because if there is a new server on the same host port, the old server should have already been dead.
- The exception thrown here is to skip the later steps of the whole regionServerReport request processing. Usually, after recording it in ServerManager, we will call the related methods in AssignmentManager to record region states. If the region server is already dead, we should not do these steps any more, so here we throw an exception to let the upper layer know that they should not continue processing any more.
There was a problem hiding this comment.
Sounds good, will add more comments to make it clear. Thanks Duo!
|
🎊 +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. |
Apache9
approved these changes
Mar 25, 2024
| // is already dead, we should not do these steps anymore, so here we throw an exception | ||
| // to let the upper layer know that they should not continue processing anymore. | ||
| final String errorMsg = "RegionServerReport ignored, could not record the server: " + sn | ||
| + " . Consider yourself dead as server with higher startcode is already registered."; |
There was a problem hiding this comment.
This doesn't need to block merging, but given the comments above, doesn't the message here seem odd? The message seems to be addressing the regionserver. But you say in the comment that this error is not sent to the regionserver and is only meant to signal upper layers to skip processing the dead server.
I wonder if we shouldn't have a new exception type or at least clarify the message (which is not just thrush but also logged)
There was a problem hiding this comment.
The exception is sent to regionserver but the purpose is bit different. So not only we don't want to process the report but also we want to let the regionserver know that it should have been dead.
Let me see how I can make this clear in the comments.
There was a problem hiding this comment.
We can not send this message to regionserver as it is already dead... I have repeated these words many times, on the jira issue and also here...
There was a problem hiding this comment.
I meant to say that master sends it as if regionserver is still alive. But in reality it is dead by the time master receives the RPC call.
There was a problem hiding this comment.
I think the comment is clear. To me the log message needs to change
There was a problem hiding this comment.
How about
final String errorMsg = "RegionServerReport received from " + sn
+ ", but another server with the same name and higher startcode is already registered, ignoring";
There was a problem hiding this comment.
Yea, that sounds great
There was a problem hiding this comment.
Sure let me make changes today.
|
💔 -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. |
|
@apurtell @bbeaudreault are you good with the latest revision of this PR? |
bbeaudreault
approved these changes
Apr 4, 2024
|
LGTM |
virajjasani
added a commit
that referenced
this pull request
Apr 5, 2024
…on inconsistencies (#5774) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
virajjasani
added a commit
that referenced
this pull request
Apr 5, 2024
…on inconsistencies (#5774) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
virajjasani
added a commit
that referenced
this pull request
Apr 5, 2024
…on inconsistencies (#5774) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
virajjasani
added a commit
that referenced
this pull request
Apr 5, 2024
…on inconsistencies (#5774) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
virajjasani
added a commit
that referenced
this pull request
Apr 5, 2024
…on inconsistencies (#5774) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
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.
Jira: HBASE-28366