Uh oh!
There was an error while loading. Please reload this page.
HBASE-28464: Make replication ZKWatcher config customizable in extens… - #5785
Open
BukrosSzabolcs wants to merge 4 commits into
Open
HBASE-28464: Make replication ZKWatcher config customizable in extens…#5785BukrosSzabolcs wants to merge 4 commits into
BukrosSzabolcs wants to merge 4 commits into
Conversation
wchevreuil
approved these changes
Mar 28, 2024
Apache-HBase
commented
Mar 28, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Mar 28, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Mar 28, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Mar 28, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Mar 28, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Mar 28, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Mar 28, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Mar 28, 2024
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Apr 2, 2024
| } | ||
| } | ||
| protected ZKClientConfig getZKClientConfig() { |
Contributor
There was a problem hiding this comment.
Better add some comments here to describe why we want to add this method? It is not used directly in the hbase code base, so maybe later other developers may decided to remove it...
ContributorAuthor
There was a problem hiding this comment.
Thanks for pointing it out, let me add an explanation.
| */ | ||
| public static RecoverableZooKeeper connect(Configuration conf, String ensemble, Watcher watcher, | ||
| final String identifier) throws IOException { | ||
| return connect(conf, ensemble, watcher, identifier, null); |
Contributor
There was a problem hiding this comment.
Why passing null instead of create an empty ZKClientConfig? In the above protected method, we create an empty ZKClientConfig instance...
ContributorAuthor
There was a problem hiding this comment.
Technically it would not make a difference, because the ZooKeeper code we end up calling would just create a new ZKClientConfig when a null is passed.
I implemented it like this to signal where the config should be modified if needed. I pass a null for use cases where the config was not passed and we expect to use the default. But I pass the instance where I expect it to be modified.
Contributor
There was a problem hiding this comment.
ZKClientConfig param is marked as Nullable in ZK code, so I think it's fine to pass null here. However you can also use the method overload without client config parameter if you don't want to customize it.
| public ZKWatcher(Configuration conf, String identifier, Abortable abortable, | ||
| boolean canCreateBaseZNode) throws IOException, ZooKeeperConnectionException { | ||
| this(conf, identifier, abortable, canCreateBaseZNode, false); | ||
| this(conf, identifier, abortable, canCreateBaseZNode, false, null); |
Apache-HBase
commented
Apr 16, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 16, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 16, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 16, 2024
💔 -1 overall
This message was automatically generated. |
Apache9
commented
Apr 17, 2024
Contributor
Mind taking a look at HBASE-28521? In general, we want to avoid leaking the internal zookeeper, so for replication, we want to use the standard connection registry API to connect to peer cluster, for compatibilitu, we will use ZKConnectionRegistry by default. But here after this change, we force hbase replication endpoint to must use zookeeper, which is not very good. I suppose we should find another way to customize the ZKWatcher creation. IIRC @anmolnar has done something related to this area, i.e, how to set zookeeper configurations in hbase configuration, with a special prefix? Thanks. |
Apache9
approved these changes
Apr 17, 2024
Apache9
left a comment
Contributor
There was a problem hiding this comment.
Anyway, checked the code, we use ReadOnlyZKClient in connection registry, not ZKWatcher, so the code is different.
Let's get this in first.
HBASE-28521 will be landed on branch-3+ first.
…ions another spotless fix
Apache-HBase
commented
Apr 17, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 17, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 17, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Apr 17, 2024
🎊 +1 overall
This message was automatically generated. |
Apache9
commented
Apr 18, 2024
Contributor
Please see #5835 |
anmolnar
commented
Apr 22, 2024
Contributor
The big difference between using Using the Connection Registry is also a very good improvement, but at the same time we should focus the above as well. My patch added the ability to set ZK system properties via |
anmolnar
commented
Apr 22, 2024
Contributor
Apache9
commented
Apr 23, 2024
Contributor
#5825 is not necessary for solving the problem for this PR, #5835 is enough, as after #5835, we are able to customize the ZKClientConfig through the Configuration map in the ReplicationPeerConfig. Let's get #5835 in and then rebase the PR here. #5825 is for solving a more general problem where we want to allow specify a remote cluster with something other than a zookeeper address and path. I think we should get it in for branch-2+, but I'm not sure whether we should get it in for branch-2.6/branch-2.5. Thanks @anmolnar ! |
anmolnar
commented
Apr 23, 2024
Contributor
BukrosSzabolcs
commented
Apr 23, 2024
ContributorAuthor
@Apache9 If I understand correctly #5835 have solved HBASE-28464 and made this PR obsolete. Why should we rebase the PR here? Am I missing something? |
Apache9
commented
Apr 24, 2024
Contributor
You are right... I was thinking that we have some special code in HBaseReplicationEndpoint for creating the ZKWatcher so we also need to modify it in this PR but actually, the creation of ZKClientConfig is inside ZKWatcher itself, so we do not need to touch HBaseReplicationEndpoint... So I think this issue can be resolved as implemented by HBASE-28529. Thanks for pointing this out. And feel free to reopen or open new issues and ping me again if HBASE-28529 can not solve all your problems. Thanks. |
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
Sign up for freeto 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.
…ions