desaikomal commented on code in PR #2137:
URL: https://github.com/apache/helix/pull/2137#discussion_r900240530
##########
helix-core/src/main/java/org/apache/helix/HelixManagerProperty.java:
##########
@@ -35,8 +36,11 @@ public class HelixManagerProperty {
private String _version;
private long _healthReportLatency;
private HelixCloudProperty _helixCloudProperty;
- private RealmAwareZkClient.RealmAwareZkConnectionConfig _zkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig
_realmAwareZkConnectionConfig;
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;
Review Comment:
Do we use Optional property indicating that it may not be set?
##########
helix-core/src/main/java/org/apache/helix/HelixManagerProperty.java:
##########
@@ -56,13 +60,15 @@ public HelixManagerProperty(Properties
helixManagerProperties, CloudConfig cloud
private HelixManagerProperty(String version, long healthReportLatency,
HelixCloudProperty helixCloudProperty,
- RealmAwareZkClient.RealmAwareZkConnectionConfig zkConnectionConfig,
- RealmAwareZkClient.RealmAwareZkClientConfig zkClientConfig) {
+ RealmAwareZkClient.RealmAwareZkConnectionConfig
realmAwareZkConnectionConfig,
+ RealmAwareZkClient.RealmAwareZkClientConfig zkClientConfig,
+ HelixZkClient.ZkConnectionConfig zkConnectionConfig) {
Review 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?
##########
helix-core/src/main/java/org/apache/helix/HelixManagerProperty.java:
##########
@@ -80,27 +86,32 @@ public long getHealthReportLatency() {
return _healthReportLatency;
}
- public RealmAwareZkClient.RealmAwareZkConnectionConfig
getZkConnectionConfig() {
- return _zkConnectionConfig;
+ public RealmAwareZkClient.RealmAwareZkConnectionConfig
getRealmAwareZkConnectionConfig() {
Review Comment:
this can break users as they used to get RealmAwareZkClient earlier, and
will need to update code to use appropriate
##########
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java:
##########
@@ -238,7 +239,6 @@ public ZKHelixManager(String clusterName, String
instanceName, InstanceType inst
HelixManagerProperty helixManagerProperty) {
validateZkConnectionSettings(zkAddress, helixManagerProperty);
- _zkAddress = zkAddress;
Review Comment:
any particular reason, zkAddress is not set here. Customer has provided as
input.
##########
helix-core/src/main/java/org/apache/helix/HelixManagerProperty.java:
##########
@@ -80,27 +86,32 @@ public long getHealthReportLatency() {
return _healthReportLatency;
}
- public RealmAwareZkClient.RealmAwareZkConnectionConfig
getZkConnectionConfig() {
- return _zkConnectionConfig;
+ public RealmAwareZkClient.RealmAwareZkConnectionConfig
getRealmAwareZkConnectionConfig() {
+ return _realmAwareZkConnectionConfig;
}
public RealmAwareZkClient.RealmAwareZkClientConfig getZkClientConfig() {
return _zkClientConfig;
}
+ public HelixZkClient.ZkConnectionConfig getZkConnectionConfig() {
+ return _zkConnectionConfig;
+ }
+
public static class Builder {
private String _version;
private long _healthReportLatency;
private HelixCloudProperty _helixCloudProperty;
- private RealmAwareZkClient.RealmAwareZkConnectionConfig
_zkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig
_realmAwareZkConnectionConfig;
private RealmAwareZkClient.RealmAwareZkClientConfig _zkClientConfig;
+ private HelixZkClient.ZkConnectionConfig _zkConnectionConfig;
public Builder() {
}
public HelixManagerProperty build() {
return new HelixManagerProperty(_version, _healthReportLatency,
_helixCloudProperty,
- _zkConnectionConfig, _zkClientConfig);
+ _realmAwareZkConnectionConfig, _zkClientConfig, _zkConnectionConfig);
Review Comment:
I agree, we should have 2 distinct methods
##########
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java:
##########
@@ -1560,30 +1560,47 @@ private RealmAwareZkClient
resolveZkClient(HelixZkClientFactory zkClientFactory,
}
/**
- * Check that not both zkAddress and ZkConnectionConfig are set.
- * If zkAddress is not given and ZkConnectionConfig is given, check that
ZkConnectionConfig has
- * a ZK path sharding key set because HelixManager must work on single-realm
mode.
+ * Check that one and only one among zkAddress, ZkConnectionConfig and
RealmAwareZkConnectionConfig are set.
+ * If zkAddress is not given directly or through ZkConnectionConfig and
RealmAwareZkConnectionConfig
+ * is given, check that RealmAwareZkConnectionConfig has a ZK path sharding
key set because
+ * HelixManager must work on single-realm mode.
* @param zkAddress
* @param helixManagerProperty
*/
private void validateZkConnectionSettings(String zkAddress,
HelixManagerProperty helixManagerProperty) {
- if (helixManagerProperty != null &&
helixManagerProperty.getZkConnectionConfig() != null) {
- if (zkAddress != null) {
- throw new HelixException(
- "ZKHelixManager: cannot have both ZkAddress and ZkConnectionConfig
set!");
- }
+ boolean hasRealmAwareZkConnectionConfig = helixManagerProperty != null
+ && helixManagerProperty.getRealmAwareZkConnectionConfig() != null;
+ boolean hasZkConnectionConfig =
+ helixManagerProperty != null &&
helixManagerProperty.getZkConnectionConfig() != null;
+ if (Stream.of(zkAddress != null, hasRealmAwareZkConnectionConfig,
hasZkConnectionConfig)
+ .filter(condition -> condition).count() != 1) {
+ throw new HelixException(
Review 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.
##########
helix-core/src/test/java/org/apache/helix/manager/zk/TestHelixManagerFactory.java:
##########
@@ -0,0 +1,70 @@
+package org.apache.helix.manager.zk;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import org.apache.helix.HelixAdmin;
+import org.apache.helix.HelixManager;
+import org.apache.helix.HelixManagerFactory;
+import org.apache.helix.HelixManagerProperty;
+import org.apache.helix.InstanceType;
+import org.apache.helix.integration.common.ZkStandAloneCMTestBase;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.zookeeper.api.client.HelixZkClient;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+/**
+ * Test {@link org.apache.helix.HelixManagerFactory}. MSDS usages are tested in
+ * {@link org.apache.helix.integration.multizk.TestMultiZkHelixJavaApis}
+ */
+public class TestHelixManagerFactory extends ZkStandAloneCMTestBase {
+ private static Logger LOG =
LoggerFactory.getLogger(TestHelixManagerFactory.class);
+ private static final String PARTICIPANT_NAME_PREFIX = "testInstance_";
+ private HelixAdmin _helixAdmin;
+
+ @BeforeClass
+ public void beforeClass() throws Exception {
+ super.beforeClass();
+ _helixAdmin = new ZKHelixAdmin.Builder().setZkAddress(ZK_ADDR).build();
+ }
+
+ @Test
+ public void testZKConnectionConfigForNonMsdsConnection() throws Exception {
+ // Create participant
+ String participantName = PARTICIPANT_NAME_PREFIX +
"testZKConnectionConfigForNonMsdsConnection";
+ _helixAdmin.addInstance(CLUSTER_NAME, new InstanceConfig(participantName));
+
+ // Create zk helix manager using HelixManagerFactory API
+ HelixManager managerParticipant = HelixManagerFactory
+ .getZKHelixManager(CLUSTER_NAME, participantName,
InstanceType.PARTICIPANT, null,
+ new HelixManagerProperty.Builder()
+ .setZkConnectionConfig(new
HelixZkClient.ZkConnectionConfig(ZK_ADDR)).build());
+ managerParticipant.connect();
+
+ // Verify manager
+ InstanceConfig instanceConfigRead =
Review Comment:
you will need multiple negative test scenario. ie. only zkAddress case, only
realmZkClientConnectconfig and combinations throwing exception etc.
##########
helix-core/src/main/java/org/apache/helix/HelixManagerProperty.java:
##########
@@ -80,27 +86,32 @@ public long getHealthReportLatency() {
return _healthReportLatency;
}
- public RealmAwareZkClient.RealmAwareZkConnectionConfig
getZkConnectionConfig() {
- return _zkConnectionConfig;
+ public RealmAwareZkClient.RealmAwareZkConnectionConfig
getRealmAwareZkConnectionConfig() {
+ return _realmAwareZkConnectionConfig;
}
public RealmAwareZkClient.RealmAwareZkClientConfig getZkClientConfig() {
return _zkClientConfig;
}
+ public HelixZkClient.ZkConnectionConfig getZkConnectionConfig() {
Review Comment:
since this is new property, you can rename this method, so that clients
don't have to change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
