Add backward compatibility support for constructing ZKHelixManager#2137
Add backward compatibility support for constructing ZKHelixManager#2137mgao0 wants to merge 2 commits intoapache:masterfrom
Conversation
mgao0
changed the title
|
|
||
| public class TestHelixManagerFactory { | ||
| private static Logger LOG = LoggerFactory.getLogger(TestHelixManagerFactory.class); | ||
|
|
There was a problem hiding this comment.
It's still work in progress. I will add the tests once they are completed.
| if (hasRealmAwareZkConnectionConfig) { | ||
| RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig = | ||
| helixManagerProperty.getZkConnectionConfig(); | ||
| helixManagerProperty.getRealmAwareZkConnectionConfig(); | ||
| if (connectionConfig.getZkRealmShardingKey() == null || connectionConfig | ||
| .getZkRealmShardingKey().isEmpty()) { | ||
| throw new HelixException( | ||
| "ZKHelixManager::ZK path sharding key must be set for ZKHelixManager! ZKHelixManager " | ||
| + "is only available on single-realm mode."); | ||
| } | ||
| _realmAwareZkConnectionConfig = connectionConfig; | ||
| } else if (hasZkConnectionConfig) { |
There was a problem hiding this comment.
Can these two be coexist? Or it is either or relationship.
There was a problem hiding this comment.
These two and zk address should not coexist, there should be only one of them. Line 1576 - 1577 is used to enforce this relationship.
mgao0
changed the title
| public HelixManagerProperty build() { | ||
| return new HelixManagerProperty(_version, _healthReportLatency, _helixCloudProperty, | ||
| _zkConnectionConfig, _zkClientConfig); | ||
| _realmAwareZkConnectionConfig, _zkClientConfig, _zkConnectionConfig); |
There was a problem hiding this comment.
Question: For this public function, shall we keep the original signature and add this as a new one?
There was a problem hiding this comment.
I agree, we should have 2 distinct methods
| private RealmAwareZkClient.RealmAwareZkClientConfig _zkClientConfig; | ||
| // This field is added for backward compatibility, when this field is set, | ||
| // _realmAwareZkConnectionConfig should not be set | ||
| private HelixZkClient.ZkConnectionConfig _zkConnectionConfig; |
There was a problem hiding this comment.
Do we use Optional property indicating that it may not be set?
| RealmAwareZkClient.RealmAwareZkClientConfig zkClientConfig) { | ||
| RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig, | ||
| RealmAwareZkClient.RealmAwareZkClientConfig zkClientConfig, | ||
| HelixZkClient.ZkConnectionConfig zkConnectionConfig) { |
There was a problem hiding this comment.
if i read your previous comment, you want either realmAwareZkConnection or regular ZkConnection, so can we not have 2 distinct constructors and not break any existing users?
|
|
||
| public RealmAwareZkClient.RealmAwareZkConnectionConfig getZkConnectionConfig() { | ||
| return _zkConnectionConfig; | ||
| public RealmAwareZkClient.RealmAwareZkConnectionConfig getRealmAwareZkConnectionConfig() { |
There was a problem hiding this comment.
this can break users as they used to get RealmAwareZkClient earlier, and will need to update code to use appropriate
| return _zkClientConfig; | ||
| } | ||
|
|
||
| public HelixZkClient.ZkConnectionConfig getZkConnectionConfig() { |
There was a problem hiding this comment.
since this is new property, you can rename this method, so that clients don't have to change.
| public HelixManagerProperty build() { | ||
| return new HelixManagerProperty(_version, _healthReportLatency, _helixCloudProperty, | ||
| _zkConnectionConfig, _zkClientConfig); | ||
| _realmAwareZkConnectionConfig, _zkClientConfig, _zkConnectionConfig); |
There was a problem hiding this comment.
I agree, we should have 2 distinct methods
| HelixManagerProperty helixManagerProperty) { | ||
| validateZkConnectionSettings(zkAddress, helixManagerProperty); | ||
|
|
||
| _zkAddress = zkAddress; |
There was a problem hiding this comment.
any particular reason, zkAddress is not set here. Customer has provided as input.
| helixManagerProperty != null && helixManagerProperty.getZkConnectionConfig() != null; | ||
| if (Stream.of(zkAddress != null, hasRealmAwareZkConnectionConfig, hasZkConnectionConfig) | ||
| .filter(condition -> condition).count() != 1) { | ||
| throw new HelixException( |
There was a problem hiding this comment.
i find this pattern a bit counter intuitive. You have 3 different choices. Typically, we would have 3 distinct constructors (eg. Integer(int), Integer(Integer), Integer(string)??)
Other thing is if customer has used a particular constructor, other properties will be null or ignored.AFAIK we don't allow update of Helix manager once constructor is called.
| managerParticipant.connect(); | ||
|
|
||
| // Verify manager | ||
| InstanceConfig instanceConfigRead = |
There was a problem hiding this comment.
you will need multiple negative test scenario. ie. only zkAddress case, only realmZkClientConnectconfig and combinations throwing exception etc.
|
Closing this PR as this is not a necessary feature, and may introduce other backward incompatibilities in order to fix it. Will evaluate the situation later and reopen if necessary. |
4 participants
Issues
Resolves #2160
Description
This commit uses a deprecated class
HelixZkClient.ZkConnectionConfigto convey the information for non-ZooScalability users in HelixManagerProperty, while ZooScalability users useRealmAwareZkClient.RealmAwareZkConnectionConfig. This commit adds some validation around there should be at least one, and only one ofHelixZkClient.ZkConnectionConfig,RealmAwareZkClient.RealmAwareZkConnectionConfig, and zk address exists, and this one field will be used as the source of information to establish connection with ZK server.Tests
Added TestHelixManagerFactory.
Also ran TestMultiZkHelixJavaApis.java to make sure this commit doesn't break current logic for ZooScalability cases.
Running
Changes that Break Backward Compatibility (Optional)
(Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)
Documentation (Optional)
(Link the GitHub wiki you added)
Commits
Code Quality
(helix-style-intellij.xml if IntelliJ IDE is used)