-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-25700 Enhance znode parent validation when add_peer #3458
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
I'm not sure whether we allow a null clusterKey? The region replica feature needs intra cluster replication? The feature does not use this class? |
The cluster key must no be null. I have met a problem in master-slave cluster replication, user misconfigured the cluster key like CLUSTER_KEY => "zk1.com:2181:/hbase " . the znode parent was end with a blank. We did not validate it when add peer, but it will cause enable_table_replication failed. |
You could see the code in ReplicationPeerManager Line 361 in 1883889
Only if the endpoint is HBaseInterClusterReplicationEndpoint we will check the peer config. Buy anyway, I think it is fine to check null here as if you need the clusterKey to be null, just do not call this method. Please add some javadoc for the methdo to mention that you should not call it with null value, if you want to it to be null, just do not call this method. Thanks. |
I think it is enough to trim the cluster key when it's not null. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
NPE will still occur while excuting ZKConfig#transformClusterKey() during cluster key validation.
IMO we should not allow empty cluster key when endpoint is HBaseInterClusterReplicationEndpoint. WDYT @Apache9 |
We have this check logic in ReplicationPeerManager. |
Oh, I think this is a bad case for spotbugs, but let's fix the pattern to not trigger the spotbugs warning? Maybe just write |
OK, I will update it. |
Just went throug the AddPeerProcedure.. AddPeerProcedure#prePeerModification In the transformClusterKey(), we access key without any validation, so here we may faceNPE. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
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.
Update for findbus check issue.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
OK, We are ensuring cluster key wont be NULL in ReplicationPeerConfigUtil#convert(ReplicationPeerConfig). Line 336 in a3ad97f
So we are good here.. |
@mokai87 Do you mind adding UT for this change? |
No problem. |
🎊 +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. |
Please fix the checkstyle issues then I will merge it. Thanks. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
|
Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Pankaj Kumar <pankajkumar@apache.org>
No description provided.