HBASE-25032 Wait for region server to become online before adding it to online servers in Master #2769
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
|
💔 -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. |
|
What about just add a flag in report duty to tell master whether the region server is ready to take regions? |
|
@Apache9 At the time of The flow is as follows: The issue we observed which led to the creation of this JIRA is a ~20min delay in RS initializing replication sources when RS is unable to connect to peer clusters because of some kerberos configuration. And since Master considers the RS online, it tries to assign regions, which fails with There are a few ways to address this issue, described here. The approach taken in this PR alters the handling of |
|
Oh, what I mean is the periodical report from RS to master, maybe it is called regionServerReport? FWIW, I think it will be better to let the region server tell master directly, so we do not need to schedule a background task on master to detect the status of a region server. Thanks. |
|
@Apache9 Okay I see. So you are suggesting a background task in RS instead of in Master. That will also require new RPC logic in Master. I am not sure if there is much of a difference in pros/cons between these two methods? Thanks |
Just a simple flag like initialized is enough? After we finish the initialization work, we set the flag to true, and regionServerReport will bring this flag to master, so master will know the region server is ready to take regions. |
|
@Apache9 There is already an Then my question is, do you know why we even do all this setup logic after we report to Master in the first place? Why not move the logic in |
|
Let me read the code again and then try to answer your questions. IIRC there is a problem on the order of register znode on zk and send reportForDuty to master, finally we decide to register to zk first but then there is a problem that if a region server only has a znode on zk but never send a reportForDuty to master, it will be in a strange state forever... |
|
Stumbled upon this one.
Agree. I commented on the jira. I think this is an init sequence problem. If we break down the RPCs into the following, I think it should work.
Looks like that should fix the problem. Doing this async stuff in master only complicates the master state machine. |
caroliney14
force-pushed
the
HBASE-25032
branch
from
11b41c7 to
40cf6a3
Compare
|
@Apache9 @bharathv Thanks for the suggestion. I looked at the code some more and I believe we only need to remove the |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
bharathv
reviewed
Jan 9, 2021
There was a problem hiding this comment.
The approach seems in the right direction to me. Basically we are marking the server live on its first report rather than at the startup RPC. The test failures in precommit seem related, mind checking?
You have to update the comments in multiple places (github doesn't let me comment inline). Places like regionServerReport(), regionServerStartup() etc.
| request.setServerCurrentTime(System.currentTimeMillis() + warningSkew * 2); | ||
| sm.regionServerStartup(request.build(), 0, "0.0.0", ia5); | ||
| } | ||
| } |
|
Oh, after checking the code, I remembered what I concerned was another problem... If we do not register our znode on zookeeper after calling regionServerStartup, there is no way for the master to mark this regionserver as dead so it will remain there forever. So I agree with @bharathv that, we should mark a region server as online after its first regionServerReport, instead of regionServerStartup, maybe we could have another map in ServerManager to record the region servers which have called regionServerStartup but no regionServerReport yet, and once there is a regionServerReport, we move it the actual active region server map and allow it to take regions. Thanks. |
caroliney14
force-pushed
the
HBASE-25032
branch
from
12f89c9 to
33c49d8
Compare
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
caroliney14
force-pushed
the
HBASE-25032
branch
from
33c49d8 to
65e6f7a
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@Apache9 I don't think I follow, can you clarify what you meant:
Why do we need another map? Isn't it enough that the server be marked as online upon its first
No need to worry about this I think, since I am not changing this logic. We are registering the znode in |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
bharathv
reviewed
Mar 24, 2021
| long now = System.currentTimeMillis(); | ||
| if ((now - lastMsg) >= msgInterval) { | ||
| // Register with the Master now that our setup is complete. | ||
| tryRegionServerReport(lastMsg, now); |
There was a problem hiding this comment.
@caroliney14 You missed my last comment on checking whether tryRegionServerReport() was successful. It can also return without sending a report successfully. Ex: RS starts before master is up. Quoting my last comment here again
You need do this only if tryRegionServerReport() is successful. It can also return without a successful report if the exception thrown is not YouAreDeadException. You may need to make that method return a boolean that tells the caller if the report was successfully sent.
So the code should be something like
boolean reportSuccess = tryRegionServerReport(...)
if (reportSuccess && !online.get()) {
....
}
+1 once that is done.
caroliney14
force-pushed
the
HBASE-25032
branch
2 times, most recently
from
42f1ec0 to
2eb52a7
Compare
|
🎊 +1 overall
This message was automatically generated. |
bharathv
approved these changes
Mar 24, 2021
There was a problem hiding this comment.
+1 pending QA. @saintstack Please sign-off on this change unless you have any other comments.
@caroliney14 Thanks for being patient and addressing the concerns, very useful contribution.
saintstack
approved these changes
Mar 24, 2021
There was a problem hiding this comment.
One suggestion else, this PR is excellent... Nice back and forth there @bharathv and @caroliney14
|
|
||
| // We registered with the Master. Go into run mode. | ||
| // Run mode. | ||
| long lastMsg = System.currentTimeMillis(); |
There was a problem hiding this comment.
How about setting this lastMsg here should be set to Long.MAX_LONG so we heartbeat immediately after the report-for-duty... so no lag before the RS is 'online'?
There was a problem hiding this comment.
I think you mean set it to LONG.MIN_LONG (or 0), since the line that checks it says if ((now - lastMsg) >= msgInterval), but yes good one 👍
There was a problem hiding this comment.
So, do in a follow-on @caroliney14 ?
There was a problem hiding this comment.
@saintstack do we have to do this one? asking because the lastMsg timestamp ultimately ends up getting stored here, not sure if it'll have any impact we care about. also, one stage of the PR build kept failing for some unknown reason when I had pushed up the change long lastMsg = 0; which is why I ended up undoing it
There was a problem hiding this comment.
No. We not have to do it. It was suggestion. You've done a mountain of great work in here already.
I'll keep an eye on it. Was thinking we heartbeat immediately but that might not be a good idea on a big cluster....
There was a problem hiding this comment.
sounds good, thank you @saintstack
caroliney14
force-pushed
the
HBASE-25032
branch
2 times, most recently
from
76c23de to
fc30075
Compare
|
💔 -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. |
caroliney14
force-pushed
the
HBASE-25032
branch
from
fc30075 to
db7c7d6
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
caroliney14
force-pushed
the
HBASE-25032
branch
from
db7c7d6 to
6558c50
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
…to online servers in Master
caroliney14
force-pushed
the
HBASE-25032
branch
from
6558c50 to
a628295
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@bharathv @saintstack the PR build passed -- can we merge this now? |
asfgit
pushed a commit
that referenced
this pull request
May 8, 2021
…s when refreshing state in some procedure implementations Revert "HBASE-25032 Wait for region server to become online before adding it to online servers in Master (#2769)" This reverts commit 1e4639d. Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.