HBASE-26172 Deprecated MasterRegistry #3566
Merged
+75
−141
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. |
bharathv
reviewed
Aug 6, 2021
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * Determine the bootstrap nodes we want to return to the client connection registry. | ||
| * <ul> | ||
| * <li>{@link #MASTER}: return masters as bootstrap nodes.</li> |
There was a problem hiding this comment.
Why do we even need this? I thought the agreement was to not let master be in the hot path? For backward compatibility sakes?
There was a problem hiding this comment.
I think at least for a small cluster, using both masters and region servers as bootstrap node is a valid requirements. And I'm not sure if someone wants to use only masters, but at least you guys used to make use of backup masters and it works fine, I think it is OK to provide a MASTER option here, not a big deal, the default value is REGIONSERVER and usually users will not change it.
There was a problem hiding this comment.
Sorry for the delay boss, I was busy last few days.
My only concern here is that it is against the agreed design principle of not putting masters inline with the connection path and exposing such a configuration might complicate things going forward as future developers might build on top of it.
Typically for small clusters, since it is impossible to run a meaningful cluster without a region server, but things can run for sometime without a master, so we can expect that at least one region server to up all the time if it is serving any load. So a registry can be served with that region server(s)? WDYT?
@saintstack / @francisliu you have any comments on this approach?
There was a problem hiding this comment.
I'm trying to figure out what the approach is. It seems to be moving Registry host to RegionServers -- which sounds good but I'm not sure how we get off the ground... bootstrap. And yeah, would be good to avoid Masters if we can.
There was a problem hiding this comment.
Looking more, appreciate offering the option but like the @bharathv comment that we should just do RS.
There was a problem hiding this comment.
It appeared that you intended to keep both MasterRegistry, RpcConnectionRegistry with masters.
Let me explain more...
The issue here, aims to remove MasterRegistry in the future, so
- Ideally RpcConnectionRegistry should cover what we have in MasterRegistry, by some configurations at least.
- We just do not want to support masters any more, so we do not need to provide configurations at server side, after MasterRegistry is removed, users can not use masters as connection registry endpoint any more.
But if we go with option 2, maybe it breaks some users, so maybe we should keep MasterRegistry and do not mark it as deprecated? And then, the RpcConnectionRegistry should better be renamed to RegionServerRegistry.
So let me conclude, there are 3 options:
- Deprecated MasterRegistry, plan a removal in 4.0.0, and let RpcConnectionRegistry still provide the ability to use masters as connection registry endpoint.
- Deprecated MasterRegistry, plan a removal in 4.0.0, but still keep RpcConnectionRegistry only use region servers as connection registry endpoint.
- Do not deprecated MasterRegistry, rename RpcConnectionRegistry to RegionServerRegistry.
I do not think we need to rename RpcConnectionRegistry to RegionServerRegistry if we still want to remove MasterRegistry, so there is no option 4.
Please let me know your choice. But if you want to choose 2 or 3, I think we'd better post a discussion email to the dev list, as it removes a feature currently in use, we need to collect more information from users.
Thanks.
There was a problem hiding this comment.
What I'm saying in my above comment is "deprecate MasterRegistry and rename RpcConnectionRegistry" (similar to option 2) and everything still works as expected for current MasterRegistry users. Details in #3566 (comment)
If we go with option 2, can you explain me what does not work for current users of MasterRegistry? If you explain how it is broken, I'm +1 on option (1). (see my last comment as to why I think it is not broken, may be I misunderstood something) #3566 (comment)
Only broken case I can think of is old client binary is not compatible with new Server binary, but that is not a problem we are worried about? Is there any other case too?
Thanks.
There was a problem hiding this comment.
There are no compatible issue between client and server...
The key difference between option 1 and 2, is that whether still support using master as registry endpoint after the removal of MasterRegistry in 4.0.0. If we still support this feature in the future, then I think a release note and a notice email to dev list is enough. If we really think we should remove this feature, then we'd better send a discussion email to dev list first. If there are no big concerns, then we can do this. If no, I think we'd better go with option 1, to let RpcConnectionRegistry still have the ability to use masters as registry endpoint.
Is this clear enough? Let me point out the key problem again, it is about whether people could still use masters as registry endpoint in 4.0.0, not about now...
Thanks.
There was a problem hiding this comment.
ya okay. I understand your concern now. Thanks for the explanation.
If we really think we should remove this feature, then we'd better send a discussion email to dev list first. If there are no big concerns, then we can do this.
+1 on this fwiw. Since the decision to inline masters in RW path was a wrong design choice, I vote to get rid of it in the long term.
Patch lgtm otherwise.
|
The failed UT is because of HBASE-26172, it needs to much time to finish... Filed HBASE-26179 for it, the PR is ready #3571 |
|
🎊 +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. |
|
Ping @bharathv , any other concerns? |
sunhelly
approved these changes
Aug 9, 2021
saintstack
reviewed
Aug 10, 2021
| @@ -49,7 +49,7 @@ | |||
| public class RpcConnectionRegistry extends AbstractRpcBasedConnectionRegistry { | |||
|
|
|||
| /** Configuration key that controls the fan out of requests **/ | |||
| public static final String HEDGED_REQS_FANOUT_KEY = "hbase.client.rpc_registry.hedged.fanout"; | |||
| public static final String HEDGED_REQS_FANOUT_KEY = "hbase.client.bootstrap.hedged.fanout"; | |||
There was a problem hiding this comment.
hbase.client.rpc_registry.hedged.fanout is a new config recently added. It is committed to a branch only currently? So it is fine changing the name? Does the config get doc'd in release notes?
There was a problem hiding this comment.
Yes, no actual release yet, this is a missing part in the previous commit, @bharathv suggest that we do not use 'rpc_registry', just use 'bootstrap', but I forgot to change this one.
No release note yet. We could mention it in the release note for this issue. Or better describe it in the ref guide? Usually you do not need to change this value.
| /** | ||
| * Determine the bootstrap nodes we want to return to the client connection registry. | ||
| * <ul> | ||
| * <li>{@link #MASTER}: return masters as bootstrap nodes.</li> |
There was a problem hiding this comment.
Looking more, appreciate offering the option but like the @bharathv comment that we should just do RS.
Apache9
changed the title
|
🎊 +1 overall
This message was automatically generated. |
|
@bharathv I've removed the configuration. Will only return region servers in getBootstrapNodes now. Thanks. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
added a commit
that referenced
this pull request
Aug 19, 2021
Signed-off-by: Xiaolin Ha <haxiaolin@apache.org> Signed-off-by: Bharath Vissapragada <bharathv@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.
No description provided.