HBASE-28425 Allow specify cluster key without zookeeper in replication #5865
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. |
|
💔 -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. |
ndimiduk
approved these changes
May 17, 2024
| * Validate the given {@code uri}. | ||
| * @throws IOException if this is not a valid connection registry URI. | ||
| */ | ||
| void validate(URI uri) throws IOException; |
There was a problem hiding this comment.
Maybe this interface should be boolean isValid(URI), and then the caller is free to throw or not.
There was a problem hiding this comment.
Throwing exception could let the upper layer know the details about why this is not valid.
There was a problem hiding this comment.
Yeah, I understand. This enforces try-catch flow control where if/else would be better. For this kind of thing, I really like the Result based API that is catching on in other languages.
Anyway, maybe it should throw something besides IOException ? IllegatStateException, for example?
There was a problem hiding this comment.
Maybe something like UnsupportedProtocol or IllegalArgument? But IllegalArgument is a RuntimeException, developers may miss to catch it and cause some fatal errors...
There was a problem hiding this comment.
I think a RuntimeException is okay. It happens often enough in the JDK libraries, where they will also add a note about it in the javadoc.
There was a problem hiding this comment.
Checked the implementation, now we have these errors
No protocol scheme
No factory registered for the scheme
For zk based registry
Empty zk server string, i.e, empty uri authority
Empty zk parent path, i.e, empty uri path
For rpc based registry
Empty bootstrap nodes, i.e, empty uri authority
In general, there are no accurate exception types for these errors, and since we may add new checks for different registry implementations in the future, I prefer we still keep the throws IOException declaration, and can file new issues to add some specific exceptions which extend HBaseIOException for these cases.
| @@ -19,6 +19,7 @@ | |||
|
|
|||
| import java.io.IOException; | |||
| import java.net.URI; | |||
| import org.apache.commons.lang3.StringUtils; | |||
There was a problem hiding this comment.
Sorry, I forget which dependencies we expose transitively. Should we be using a shaded version of this class?
There was a problem hiding this comment.
Let me check if we have commons-lang3 shaded.
There was a problem hiding this comment.
We do not shade commons-lang3 in hbase-thirdparty
There was a problem hiding this comment.
Okay thanks for checking. I wonder if we should.
There was a problem hiding this comment.
Commons-lang3 is not likely to introduce big conflicts. It changed its package name to commons-lang3 from commons-lang when introducing breaking changes.
hbase-common/src/main/java/org/apache/hadoop/hbase/util/ReservoirSample.java
Outdated
Show resolved
Hide resolved
| @@ -86,6 +86,14 @@ public class TestVerifyReplication extends TestReplicationBase { | |||
| @Rule | |||
| public TestName name = new TestName(); | |||
|
|
|||
| @Override | |||
| protected String getClusterKey(HBaseTestingUtil util) throws Exception { | |||
| // TODO: VerifyReplication does not support connection uri yet, so here we need to use cluster | |||
There was a problem hiding this comment.
Is there an issue filed for this TODO?
hbase-replication/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeers.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/replication/HBaseReplicationEndpoint.java
Outdated
Show resolved
Hide resolved
hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtil.java
Show resolved
Hide resolved
hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtil.java
Show resolved
Hide resolved
...st/java/org/apache/hadoop/hbase/replication/regionserver/TestGlobalReplicationThrottler.java
Show resolved
Hide resolved
hbase-testing-util/src/main/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
Show resolved
Hide resolved
ndimiduk
approved these changes
May 17, 2024
| * Validate the given {@code uri}. | ||
| * @throws IOException if this is not a valid connection registry URI. | ||
| */ | ||
| void validate(URI uri) throws IOException; |
There was a problem hiding this comment.
Yeah, I understand. This enforces try-catch flow control where if/else would be better. For this kind of thing, I really like the Result based API that is catching on in other languages.
Anyway, maybe it should throw something besides IOException ? IllegatStateException, for example?
| @@ -19,6 +19,7 @@ | |||
|
|
|||
| import java.io.IOException; | |||
| import java.net.URI; | |||
| import org.apache.commons.lang3.StringUtils; | |||
hbase-common/src/main/java/org/apache/hadoop/hbase/util/ReservoirSample.java
Outdated
Show resolved
Hide resolved
| @@ -86,6 +86,14 @@ public class TestVerifyReplication extends TestReplicationBase { | |||
| @Rule | |||
| public TestName name = new TestName(); | |||
|
|
|||
| @Override | |||
| protected String getClusterKey(HBaseTestingUtil util) throws Exception { | |||
| // TODO: VerifyReplication does not support connection uri yet, so here we need to use cluster | |||
hbase-replication/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeers.java
Show resolved
Hide resolved
| try (Connection conn = ConnectionFactory.createConnection(connectionUri, conf); | ||
| Admin admin = conn.getAdmin()) { | ||
| peerClusterId = | ||
| admin.getClusterMetrics(EnumSet.of(ClusterMetrics.Option.CLUSTER_ID)).getClusterId(); |
There was a problem hiding this comment.
That would be nice, but I guess not urgent.
hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtil.java
Show resolved
Hide resolved
| } catch (IOException | KeeperException e) { | ||
| // we just want to check whether we will replicate to the same cluster, so if we get an error | ||
| // while getting the cluster id of the peer cluster, it means we are not connecting to | ||
| // ourselves, as we are still alive. So here we just log the error and continue |
|
🎊 +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. |
|
Going to merge this today if no other concerns. |
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.