-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-24743 Reject to add a peer which replicate to itself earlier #2071
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
🎊 +1 overall
This message was automatically generated. |
wchevreuil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, after addressing checkstyles issues.
...-server/src/main/java/org/apache/hadoop/hbase/master/replication/ReplicationPeerManager.java
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
...-server/src/main/java/org/apache/hadoop/hbase/master/replication/ReplicationPeerManager.java
Show resolved
Hide resolved
| return false; | ||
| } | ||
| }); | ||
| peerClusterId = ZKClusterId.readClusterIdZNode(zkWatcher); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ReplicationPeerManager is colocated with master, right? you can use master.getClusterId() without doing all this? (that is cached, so saves a ZK round trip).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not right. This fetch peer cluster id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the changed code in hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
...-server/src/main/java/org/apache/hadoop/hbase/master/replication/ReplicationPeerManager.java
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
bharathv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last set of questions, then +1.
...-server/src/main/java/org/apache/hadoop/hbase/master/replication/ReplicationPeerManager.java
Show resolved
Hide resolved
| private void checkPeerConfig(ReplicationPeerConfig peerConfig) throws DoNotRetryIOException { | ||
| String replicationEndpointImpl = peerConfig.getReplicationEndpointImpl(); | ||
| boolean checkClusterKey = true; | ||
| ReplicationEndpoint endpoint = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit = null is redundant.
...-server/src/main/java/org/apache/hadoop/hbase/master/replication/ReplicationPeerManager.java
Show resolved
Hide resolved
...-server/src/main/java/org/apache/hadoop/hbase/master/replication/ReplicationPeerManager.java
Show resolved
Hide resolved
| // Create the peer cluster config for get peer cluster id | ||
| Configuration peerConf = HBaseConfiguration.createClusterConf(conf, clusterKey); | ||
| ZKWatcher zkWatcher = | ||
| new ZKWatcher(peerConf, this + "check-peer-cluster-key", new Abortable() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like ZKWatcher is null-safe with abortable. Since we are not actually relying on it, just pass null?
...-server/src/main/java/org/apache/hadoop/hbase/master/replication/ReplicationPeerManager.java
Show resolved
Hide resolved
...-server/src/main/java/org/apache/hadoop/hbase/master/replication/ReplicationPeerManager.java
Show resolved
Hide resolved
|
💔 -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. |
…2071) Signed-off-by: Wellington Chevreuil <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
|
@infraio |
…pache#2071) Signed-off-by: Wellington Chevreuil <[email protected]> Signed-off-by: Bharath Vissapragada <[email protected]>
…rlier (apache#2071)" This reverts commit 5db3ec2. TestReplicationAdmin and TestReplicationShell are broken on branch-2 and master respectively
No description provided.