You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/07/29 07:38:07 UTC
[GitHub] [helix] narendly opened a new pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
narendly opened a new pull request #1183:
URL: https://github.com/apache/helix/pull/1183
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1182
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
Previously, there wasn't a way to use HelixManager with a custom routing data ZK connection config. This new way of constructing HelixManager allows users to use HelixManager with a custom RealmAwareZkConnectionConfig.
### Tests
- [x] The following tests are written for this issue:
testZKHelixManager
- [x] The following is the result of the "mvn test" command on the appropriate module:
### Commits
- [x] My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation (Optional)
- [ ] In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Code Quality
- [x] My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r462435090
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerFactory.java
##########
@@ -65,4 +68,38 @@ public static HelixManager getZKHelixManager(String clusterName, String instance
return new ZKHelixManager(clusterName, instanceName, type, zkAddr, stateListener);
}
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given. If a proper
+ * ZkConnectionConfig. HelixManagerProperty given must contain a valid ZkConnectionConfig.
Review comment:
The doc is not written in a coherent way.
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerFactory.java
##########
@@ -65,4 +68,38 @@ public static HelixManager getZKHelixManager(String clusterName, String instance
return new ZKHelixManager(clusterName, instanceName, type, zkAddr, stateListener);
}
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given. If a proper
+ * ZkConnectionConfig. HelixManagerProperty given must contain a valid ZkConnectionConfig.
+ * @param clusterName
+ * @param instanceName
+ * @param type
+ * @param stateListener
+ * @param helixManagerProperty must contain a valid ZkConnectionConfig
+ * @return
+ */
+ public static HelixManager getZKHelixManager(String clusterName, String instanceName,
+ InstanceType type, HelixManagerStateListener stateListener,
+ HelixManagerProperty helixManagerProperty) {
+ return new ZKHelixManager(clusterName, instanceName, type, null, stateListener,
+ helixManagerProperty);
+ }
+
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given. If a proper
+ * ZkConnectionConfig is given in HelixManagerProperty, zkAddr field will be overriden.
+ * @param clusterName
+ * @param instanceName
+ * @param type
+ * @param zkAddr will be overriden if a valid ZkConnectionConfig is given in helixManagerProperty
Review comment:
This flexibility is a bit weird. I don't know whether users would follow it. Can we force them to have only one set up instead of using the override, like to validate the input.
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerProperty.java
##########
@@ -20,54 +20,82 @@
*/
import java.util.Properties;
+
import org.apache.helix.model.CloudConfig;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+
/**
- * Hold Helix manager properties. The manager properties further hold Helix cloud properties
- * and some other properties specific for the manager.
+ * HelixManagerProperty is a general property/config object used for HelixManager creation.
*/
public class HelixManagerProperty {
private static final Logger LOG = LoggerFactory.getLogger(HelixManagerProperty.class.getName());
private String _version;
private long _healthReportLatency;
private HelixCloudProperty _helixCloudProperty;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig _zkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig _zkClientConfig;
/**
+ * ** Deprecated - HelixManagerProperty should be a general property/config object used for
+ * HelixManager creation, not tied only to Properties or CloudConfig **
+ *
* Initialize Helix manager property with default value
* @param helixManagerProperties helix manager related properties input as a map
* @param cloudConfig cloudConfig read from Zookeeper
*/
+ @Deprecated
public HelixManagerProperty(Properties helixManagerProperties, CloudConfig cloudConfig) {
_helixCloudProperty = new HelixCloudProperty(cloudConfig);
setVersion(helixManagerProperties.getProperty(SystemPropertyKeys.HELIX_MANAGER_VERSION));
setHealthReportLatency(
helixManagerProperties.getProperty(SystemPropertyKeys.PARTICIPANT_HEALTH_REPORT_LATENCY));
}
+ public HelixManagerProperty() {
Review comment:
The original plan was to let users only get the manager property through `getHelixManagerProperty` with default value, and then allow them to make modification to override the fields in it. I remember there was some concern about users accidentally changing it. We can revisit the assumption though.
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerProperty.java
##########
@@ -20,54 +20,82 @@
*/
import java.util.Properties;
+
import org.apache.helix.model.CloudConfig;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+
/**
- * Hold Helix manager properties. The manager properties further hold Helix cloud properties
- * and some other properties specific for the manager.
+ * HelixManagerProperty is a general property/config object used for HelixManager creation.
*/
public class HelixManagerProperty {
private static final Logger LOG = LoggerFactory.getLogger(HelixManagerProperty.class.getName());
private String _version;
private long _healthReportLatency;
private HelixCloudProperty _helixCloudProperty;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig _zkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig _zkClientConfig;
/**
+ * ** Deprecated - HelixManagerProperty should be a general property/config object used for
+ * HelixManager creation, not tied only to Properties or CloudConfig **
+ *
* Initialize Helix manager property with default value
* @param helixManagerProperties helix manager related properties input as a map
* @param cloudConfig cloudConfig read from Zookeeper
*/
+ @Deprecated
public HelixManagerProperty(Properties helixManagerProperties, CloudConfig cloudConfig) {
_helixCloudProperty = new HelixCloudProperty(cloudConfig);
setVersion(helixManagerProperties.getProperty(SystemPropertyKeys.HELIX_MANAGER_VERSION));
setHealthReportLatency(
Review comment:
fyi, the reason that we have a couple of fields like "version", "latency" here is just to give an example of how to migrate system properties to Helix manager property. A "TODO" should have been added here for completeness in future work.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r464514761
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerFactory.java
##########
@@ -65,4 +64,39 @@ public static HelixManager getZKHelixManager(String clusterName, String instance
return new ZKHelixManager(clusterName, instanceName, type, zkAddr, stateListener);
}
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given.
+ * HelixManagerProperty given must contain a valid ZkConnectionConfig.
+ * @param clusterName
+ * @param instanceName
+ * @param type
+ * @param stateListener
+ * @param helixManagerProperty must contain a valid ZkConnectionConfig
+ * @return
+ */
+ public static HelixManager getZKHelixManager(String clusterName, String instanceName,
+ InstanceType type, HelixManagerStateListener stateListener,
+ HelixManagerProperty helixManagerProperty) {
+ return new ZKHelixManager(clusterName, instanceName, type, null, stateListener,
+ helixManagerProperty);
+ }
+
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given.
+ * NOTE: if both zkAddr and a valid ZkConnectionConfig are given in HelixManagerProperty, the
+ * instantiation will fail - only one is required.
+ * @param clusterName
+ * @param instanceName
+ * @param type
+ * @param zkAddr
+ * @param stateListener
+ * @param helixManagerProperty
+ * @return
+ */
+ public static HelixManager getZKHelixManager(String clusterName, String instanceName,
+ InstanceType type, String zkAddr, HelixManagerStateListener stateListener,
+ HelixManagerProperty helixManagerProperty) {
Review comment:
Have you considered: what if a user needs to provide both `zkAddr` and `helixManagerProperty`?
At any rate, we could add this constructor later when we absolutely need to. For the sake of keeping this moving forward, I'll remove this `getZkHelixManager` method for now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r461981674
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerProperty.java
##########
@@ -20,54 +20,82 @@
*/
import java.util.Properties;
+
import org.apache.helix.model.CloudConfig;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+
/**
- * Hold Helix manager properties. The manager properties further hold Helix cloud properties
- * and some other properties specific for the manager.
+ * HelixManagerProperty is a general property/config object used for HelixManager creation.
*/
public class HelixManagerProperty {
private static final Logger LOG = LoggerFactory.getLogger(HelixManagerProperty.class.getName());
private String _version;
private long _healthReportLatency;
private HelixCloudProperty _helixCloudProperty;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig _zkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig _zkClientConfig;
/**
+ * ** Deprecated - HelixManagerProperty should be a general property/config object used for
+ * HelixManager creation, not tied only to Properties or CloudConfig **
+ *
* Initialize Helix manager property with default value
* @param helixManagerProperties helix manager related properties input as a map
* @param cloudConfig cloudConfig read from Zookeeper
*/
+ @Deprecated
public HelixManagerProperty(Properties helixManagerProperties, CloudConfig cloudConfig) {
_helixCloudProperty = new HelixCloudProperty(cloudConfig);
setVersion(helixManagerProperties.getProperty(SystemPropertyKeys.HELIX_MANAGER_VERSION));
setHealthReportLatency(
helixManagerProperties.getProperty(SystemPropertyKeys.PARTICIPANT_HEALTH_REPORT_LATENCY));
}
+ public HelixManagerProperty() {
Review comment:
I don't like that idea - it loses flexibility. What is the benefit of making this package-private?
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -225,9 +226,24 @@ public ZKHelixManager(String clusterName, String instanceName, InstanceType inst
String zkAddress, HelixManagerStateListener stateListener,
HelixManagerProperty helixManagerProperty) {
Review comment:
The documentation makes it clear that zkAddress given will be overriden if a valid ZkConnectionConfig is supplied.
We cannot remove zkAddress as that would break backward-compatibility.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -225,9 +226,24 @@ public ZKHelixManager(String clusterName, String instanceName, InstanceType inst
String zkAddress, HelixManagerStateListener stateListener,
HelixManagerProperty helixManagerProperty) {
Review comment:
The documentation makes it clear that zkAddress given will be overriden if a valid ZkConnectionConfig is supplied.
We cannot remove zkAddress as that would break backward-compatibility. getMetadataStoreConnectionString should return the value of _zkAddress as it did before, otherwise, that would break backward-compatibility. Also I am not a fan of the idea of modifying this field in mid-flight because that might cause inconsistency. It is either set of not set, not something that should be updated or modified after-the-fact.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -225,9 +226,24 @@ public ZKHelixManager(String clusterName, String instanceName, InstanceType inst
String zkAddress, HelixManagerStateListener stateListener,
HelixManagerProperty helixManagerProperty) {
Review comment:
@jiajunwang Then we could play it safe by adding a check so that we do not allow instantiation of a ZkHelixManager instance when both zkAddress and ZkConnectionConfig are set. How does that sound?
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerProperty.java
##########
@@ -20,54 +20,82 @@
*/
import java.util.Properties;
+
import org.apache.helix.model.CloudConfig;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+
/**
- * Hold Helix manager properties. The manager properties further hold Helix cloud properties
- * and some other properties specific for the manager.
+ * HelixManagerProperty is a general property/config object used for HelixManager creation.
*/
public class HelixManagerProperty {
private static final Logger LOG = LoggerFactory.getLogger(HelixManagerProperty.class.getName());
private String _version;
private long _healthReportLatency;
private HelixCloudProperty _helixCloudProperty;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig _zkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig _zkClientConfig;
/**
+ * ** Deprecated - HelixManagerProperty should be a general property/config object used for
+ * HelixManager creation, not tied only to Properties or CloudConfig **
+ *
* Initialize Helix manager property with default value
* @param helixManagerProperties helix manager related properties input as a map
* @param cloudConfig cloudConfig read from Zookeeper
*/
+ @Deprecated
public HelixManagerProperty(Properties helixManagerProperties, CloudConfig cloudConfig) {
_helixCloudProperty = new HelixCloudProperty(cloudConfig);
setVersion(helixManagerProperties.getProperty(SystemPropertyKeys.HELIX_MANAGER_VERSION));
setHealthReportLatency(
helixManagerProperties.getProperty(SystemPropertyKeys.PARTICIPANT_HEALTH_REPORT_LATENCY));
}
+ public HelixManagerProperty() {
Review comment:
Then that in and of itself is a verbose way of doing things. At that point, we should eliminate and just add a HelixProperty Builder that allows creation of HelixManagerProperty instance with final fields.
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerProperty.java
##########
@@ -20,54 +20,82 @@
*/
import java.util.Properties;
+
import org.apache.helix.model.CloudConfig;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+
/**
- * Hold Helix manager properties. The manager properties further hold Helix cloud properties
- * and some other properties specific for the manager.
+ * HelixManagerProperty is a general property/config object used for HelixManager creation.
*/
public class HelixManagerProperty {
private static final Logger LOG = LoggerFactory.getLogger(HelixManagerProperty.class.getName());
private String _version;
private long _healthReportLatency;
private HelixCloudProperty _helixCloudProperty;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig _zkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig _zkClientConfig;
/**
+ * ** Deprecated - HelixManagerProperty should be a general property/config object used for
+ * HelixManager creation, not tied only to Properties or CloudConfig **
+ *
* Initialize Helix manager property with default value
* @param helixManagerProperties helix manager related properties input as a map
* @param cloudConfig cloudConfig read from Zookeeper
*/
+ @Deprecated
public HelixManagerProperty(Properties helixManagerProperties, CloudConfig cloudConfig) {
_helixCloudProperty = new HelixCloudProperty(cloudConfig);
setVersion(helixManagerProperties.getProperty(SystemPropertyKeys.HELIX_MANAGER_VERSION));
setHealthReportLatency(
helixManagerProperties.getProperty(SystemPropertyKeys.PARTICIPANT_HEALTH_REPORT_LATENCY));
}
+ public HelixManagerProperty() {
Review comment:
Then that in and of itself is a verbose way of doing things and sounds like a code smell. At that point, we should eliminate and just add a HelixProperty Builder that allows creation of HelixManagerProperty instance with final fields.
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerProperty.java
##########
@@ -20,54 +20,82 @@
*/
import java.util.Properties;
+
import org.apache.helix.model.CloudConfig;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+
/**
- * Hold Helix manager properties. The manager properties further hold Helix cloud properties
- * and some other properties specific for the manager.
+ * HelixManagerProperty is a general property/config object used for HelixManager creation.
*/
public class HelixManagerProperty {
private static final Logger LOG = LoggerFactory.getLogger(HelixManagerProperty.class.getName());
private String _version;
private long _healthReportLatency;
private HelixCloudProperty _helixCloudProperty;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig _zkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig _zkClientConfig;
/**
+ * ** Deprecated - HelixManagerProperty should be a general property/config object used for
+ * HelixManager creation, not tied only to Properties or CloudConfig **
+ *
* Initialize Helix manager property with default value
* @param helixManagerProperties helix manager related properties input as a map
* @param cloudConfig cloudConfig read from Zookeeper
*/
+ @Deprecated
public HelixManagerProperty(Properties helixManagerProperties, CloudConfig cloudConfig) {
_helixCloudProperty = new HelixCloudProperty(cloudConfig);
setVersion(helixManagerProperties.getProperty(SystemPropertyKeys.HELIX_MANAGER_VERSION));
setHealthReportLatency(
helixManagerProperties.getProperty(SystemPropertyKeys.PARTICIPANT_HEALTH_REPORT_LATENCY));
}
+ public HelixManagerProperty() {
Review comment:
Then that in and of itself is a verbose way of doing things and sounds like a code smell. At that point, we should eliminate and just add a HelixProperty Builder that allows creation of HelixManagerProperty instance with final fields.
In my opinion, there's little value-add making that change here. What's the harm if the HelixManagerProperty instance is mutable?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r463341779
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -111,7 +111,7 @@
public static final int DEFAULT_MAX_DISCONNECT_THRESHOLD = 600; // Default to be a large number
private static final int DEFAULT_WAIT_CONNECTED_TIMEOUT = 10 * 1000; // wait until connected for up to 10 seconds.
- protected final String _zkAddress;
+ protected String _zkAddress;
Review comment:
No need to remove final right?
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerFactory.java
##########
@@ -65,4 +64,39 @@ public static HelixManager getZKHelixManager(String clusterName, String instance
return new ZKHelixManager(clusterName, instanceName, type, zkAddr, stateListener);
}
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given.
+ * HelixManagerProperty given must contain a valid ZkConnectionConfig.
+ * @param clusterName
+ * @param instanceName
+ * @param type
+ * @param stateListener
+ * @param helixManagerProperty must contain a valid ZkConnectionConfig
+ * @return
+ */
+ public static HelixManager getZKHelixManager(String clusterName, String instanceName,
+ InstanceType type, HelixManagerStateListener stateListener,
+ HelixManagerProperty helixManagerProperty) {
+ return new ZKHelixManager(clusterName, instanceName, type, null, stateListener,
+ helixManagerProperty);
+ }
+
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given.
+ * NOTE: if both zkAddr and a valid ZkConnectionConfig are given in HelixManagerProperty, the
+ * instantiation will fail - only one is required.
+ * @param clusterName
+ * @param instanceName
+ * @param type
+ * @param zkAddr
+ * @param stateListener
+ * @param helixManagerProperty
+ * @return
+ */
+ public static HelixManager getZKHelixManager(String clusterName, String instanceName,
+ InstanceType type, String zkAddr, HelixManagerStateListener stateListener,
+ HelixManagerProperty helixManagerProperty) {
Review comment:
I think we shall not be blocked on this zkAddress argument. It can be discussed later.
How about this, to ensure we are not introducing more confusing, we don't this specific method. So for this class, either you give a zkAddress to get HM or you give a HelixManagerProperty to get the HM.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -1464,4 +1471,48 @@ private RealmAwareZkClient resolveZkClient(HelixZkClientFactory zkClientFactory,
private String buildShardingKey() {
return _clusterName.charAt(0) == '/' ? _clusterName : "/" + _clusterName;
}
+
+ /**
+ * 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.
+ * @param zkAddress
+ * @param helixManagerProperty
+ */
+ private void validateZkConnectionSettings(String zkAddress,
+ HelixManagerProperty helixManagerProperty) {
+ if (helixManagerProperty != null && helixManagerProperty.getZkConnectionConfig() != null) {
+ if (zkAddress != null && !zkAddress.isEmpty()) {
+ throw new HelixException(
+ "ZKHelixManager: cannot have both ZkAddress and ZkConnectionConfig set!");
+ }
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
+ helixManagerProperty.getZkConnectionConfig();
+ 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;
+ }
+ }
+
+ /**
+ * Resolve ZK connection info for logging purposes.
+ * @return
+ */
+ private String getZkConnectionInfo() {
+ String zkConnectionInfo;
+ if (_zkAddress == null) {
+ if (_helixManagerProperty != null && _helixManagerProperty.getZkConnectionConfig() != null) {
+ zkConnectionInfo = _helixManagerProperty.getZkConnectionConfig().toString();
+ } else {
+ zkConnectionInfo = "ZkAddr and ZkConnectionConfig are null!";
Review comment:
nit, I think just return "None" is enough. Since otherwise, the log string will look strange.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -1464,4 +1471,48 @@ private RealmAwareZkClient resolveZkClient(HelixZkClientFactory zkClientFactory,
private String buildShardingKey() {
return _clusterName.charAt(0) == '/' ? _clusterName : "/" + _clusterName;
}
+
+ /**
+ * 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.
+ * @param zkAddress
+ * @param helixManagerProperty
+ */
+ private void validateZkConnectionSettings(String zkAddress,
+ HelixManagerProperty helixManagerProperty) {
+ if (helixManagerProperty != null && helixManagerProperty.getZkConnectionConfig() != null) {
+ if (zkAddress != null && !zkAddress.isEmpty()) {
Review comment:
Let's be strict here, I think even empty string is not allowed. The zkAddress shall be completely untouched.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -131,6 +131,7 @@
private int _reportLatency;
protected RealmAwareZkClient _zkclient;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig _realmAwareZkConnectionConfig;
Review comment:
Can we make this field final too? Since it is an input config.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r463224703
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -131,6 +131,7 @@
private int _reportLatency;
protected RealmAwareZkClient _zkclient;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig _realmAwareZkConnectionConfig;
Review comment:
That goes against the design of "RealmAware"ZkClient/configs. They are not tied to a single zk, and therefore we should not have a ZK field.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r463239096
##########
File path: helix-core/src/main/java/org/apache/helix/HelixPropertyFactory.java
##########
@@ -88,8 +88,9 @@ private CloudConfig getCloudConfig(String zkAddress, String clusterName) {
CloudConfig cloudConfig;
RealmAwareZkClient dedicatedZkClient = null;
try {
- if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED)) {
- // If the multi ZK config is enabled, use multi-realm mode with DedicatedZkClient
+ if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED) || zkAddress == null) {
Review comment:
Could you make it in a function? So for example, if later you have other conditions, you'll still need to add it in all related places?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r463217335
##########
File path: helix-core/src/main/java/org/apache/helix/HelixPropertyFactory.java
##########
@@ -88,8 +88,9 @@ private CloudConfig getCloudConfig(String zkAddress, String clusterName) {
CloudConfig cloudConfig;
RealmAwareZkClient dedicatedZkClient = null;
try {
- if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED)) {
- // If the multi ZK config is enabled, use multi-realm mode with DedicatedZkClient
+ if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED) || zkAddress == null) {
Review comment:
Seems we will need to follow this patten:
`if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED) || zkAddr == null)`
in a couple of different places. Is there a better way to represent? The first one is self explanatory, but the second one does not look straightforward. The doc for explanation is also in another file.
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerFactory.java
##########
@@ -19,19 +19,18 @@
* under the License.
*/
-/**
- * factory that creates cluster managers
- *
- * for zk-based cluster managers, the getZKXXX(..zkClient) that takes a zkClient parameter
- * are intended for session expiry test purpose
- */
import org.apache.helix.manager.zk.HelixManagerStateListener;
import org.apache.helix.manager.zk.ZKHelixManager;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+
/**
* Obtain one of a set of Helix cluster managers, organized by the backing system.
+ * factory that creates cluster managers
+ * *
+ * * for zk-based cluster managers, the getZKXXX(..zkClient) that takes a zkClient parameter
+ * * are intended for session expiry test purpose
Review comment:
This description is not quite easy to follow.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r463222910
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerFactory.java
##########
@@ -19,19 +19,18 @@
* under the License.
*/
-/**
- * factory that creates cluster managers
- *
- * for zk-based cluster managers, the getZKXXX(..zkClient) that takes a zkClient parameter
- * are intended for session expiry test purpose
- */
import org.apache.helix.manager.zk.HelixManagerStateListener;
import org.apache.helix.manager.zk.ZKHelixManager;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+
/**
* Obtain one of a set of Helix cluster managers, organized by the backing system.
+ * factory that creates cluster managers
+ * *
+ * * for zk-based cluster managers, the getZKXXX(..zkClient) that takes a zkClient parameter
+ * * are intended for session expiry test purpose
Review comment:
@zhangmeng916
Objectively, this comment is not very helpful to me - one tip when giving reviews & comments: either make your comment concrete by clarifying what's not easy to follow, or suggest an alternative?
With that said, I'm just going to remove the second block since it doesn't apply here. I think the second paragraph that starts with "for zk-based..." is a legacy comment that no longer applies to this class.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r464516203
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -131,6 +131,7 @@
private int _reportLatency;
protected RealmAwareZkClient _zkclient;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig _realmAwareZkConnectionConfig;
Review comment:
It is not given via ZkHelixManager's constructor, meaning it could be null. It comes from HelixManagerProperty as an optional field. So we cannot make this final.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r463222910
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerFactory.java
##########
@@ -19,19 +19,18 @@
* under the License.
*/
-/**
- * factory that creates cluster managers
- *
- * for zk-based cluster managers, the getZKXXX(..zkClient) that takes a zkClient parameter
- * are intended for session expiry test purpose
- */
import org.apache.helix.manager.zk.HelixManagerStateListener;
import org.apache.helix.manager.zk.ZKHelixManager;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+
/**
* Obtain one of a set of Helix cluster managers, organized by the backing system.
+ * factory that creates cluster managers
+ * *
+ * * for zk-based cluster managers, the getZKXXX(..zkClient) that takes a zkClient parameter
+ * * are intended for session expiry test purpose
Review comment:
@zhangmeng916
Objectively, this comment is not very helpful to me - one tip when giving reviews & comments: either make your comment concrete by clarifying what's not easy to follow, or suggest an alternative?
With that said, I'm just going to remove the second block since it doesn't apply here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r463225530
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerFactory.java
##########
@@ -65,4 +64,39 @@ public static HelixManager getZKHelixManager(String clusterName, String instance
return new ZKHelixManager(clusterName, instanceName, type, zkAddr, stateListener);
}
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given.
+ * HelixManagerProperty given must contain a valid ZkConnectionConfig.
+ * @param clusterName
+ * @param instanceName
+ * @param type
+ * @param stateListener
+ * @param helixManagerProperty must contain a valid ZkConnectionConfig
+ * @return
+ */
+ public static HelixManager getZKHelixManager(String clusterName, String instanceName,
+ InstanceType type, HelixManagerStateListener stateListener,
+ HelixManagerProperty helixManagerProperty) {
+ return new ZKHelixManager(clusterName, instanceName, type, null, stateListener,
+ helixManagerProperty);
+ }
+
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given.
+ * NOTE: if both zkAddr and a valid ZkConnectionConfig are given in HelixManagerProperty, the
+ * instantiation will fail - only one is required.
+ * @param clusterName
+ * @param instanceName
+ * @param type
+ * @param zkAddr
+ * @param stateListener
+ * @param helixManagerProperty
+ * @return
+ */
+ public static HelixManager getZKHelixManager(String clusterName, String instanceName,
+ InstanceType type, String zkAddr, HelixManagerStateListener stateListener,
+ HelixManagerProperty helixManagerProperty) {
Review comment:
RealmAware zk config should not have a field called ZkAddress. That's against the design of RealmAware API.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r463194146
##########
File path: helix-core/src/main/java/org/apache/helix/HelixPropertyFactory.java
##########
@@ -33,7 +33,7 @@
import org.slf4j.LoggerFactory;
-/**
+/***
Review comment:
extra *
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerFactory.java
##########
@@ -65,4 +64,39 @@ public static HelixManager getZKHelixManager(String clusterName, String instance
return new ZKHelixManager(clusterName, instanceName, type, zkAddr, stateListener);
}
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given.
+ * HelixManagerProperty given must contain a valid ZkConnectionConfig.
+ * @param clusterName
+ * @param instanceName
+ * @param type
+ * @param stateListener
+ * @param helixManagerProperty must contain a valid ZkConnectionConfig
+ * @return
+ */
+ public static HelixManager getZKHelixManager(String clusterName, String instanceName,
+ InstanceType type, HelixManagerStateListener stateListener,
+ HelixManagerProperty helixManagerProperty) {
+ return new ZKHelixManager(clusterName, instanceName, type, null, stateListener,
+ helixManagerProperty);
+ }
+
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given.
+ * NOTE: if both zkAddr and a valid ZkConnectionConfig are given in HelixManagerProperty, the
+ * instantiation will fail - only one is required.
+ * @param clusterName
+ * @param instanceName
+ * @param type
+ * @param zkAddr
+ * @param stateListener
+ * @param helixManagerProperty
+ * @return
+ */
+ public static HelixManager getZKHelixManager(String clusterName, String instanceName,
+ InstanceType type, String zkAddr, HelixManagerStateListener stateListener,
+ HelixManagerProperty helixManagerProperty) {
Review comment:
In addition, if we do so, then the HelixPropertyFactory can call the property builder to build a property even for the existing getHelixProperty API, right?
So it would be clean,
1. Callers only call factory methods to get property.
2. The factory calls the property builder to get the new object.
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerFactory.java
##########
@@ -65,4 +64,39 @@ public static HelixManager getZKHelixManager(String clusterName, String instance
return new ZKHelixManager(clusterName, instanceName, type, zkAddr, stateListener);
}
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given.
+ * HelixManagerProperty given must contain a valid ZkConnectionConfig.
+ * @param clusterName
+ * @param instanceName
+ * @param type
+ * @param stateListener
+ * @param helixManagerProperty must contain a valid ZkConnectionConfig
+ * @return
+ */
+ public static HelixManager getZKHelixManager(String clusterName, String instanceName,
+ InstanceType type, HelixManagerStateListener stateListener,
+ HelixManagerProperty helixManagerProperty) {
+ return new ZKHelixManager(clusterName, instanceName, type, null, stateListener,
+ helixManagerProperty);
+ }
+
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given.
+ * NOTE: if both zkAddr and a valid ZkConnectionConfig are given in HelixManagerProperty, the
+ * instantiation will fail - only one is required.
+ * @param clusterName
+ * @param instanceName
+ * @param type
+ * @param zkAddr
+ * @param stateListener
+ * @param helixManagerProperty
+ * @return
+ */
+ public static HelixManager getZKHelixManager(String clusterName, String instanceName,
+ InstanceType type, String zkAddr, HelixManagerStateListener stateListener,
+ HelixManagerProperty helixManagerProperty) {
Review comment:
Can we get rid of this constructor by allowing setup zkAddr in the HelixManagerProperty or in the RealmAwareZkConnectionConfig?
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -131,6 +131,7 @@
private int _reportLatency;
protected RealmAwareZkClient _zkclient;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig _realmAwareZkConnectionConfig;
Review comment:
As I pinged you in slack, if we can let RealmAwareZkConnectionConfig backward supports _zkAddress, then we can safe tons of validations and branches code in this class.
Moreover, we can simplify the code following this principle,
1. new APIs only accept RealmAwareZkConnectionConfig, we don't use zkAddress directly.
2. for older API, we create default RealmAwareZkConnectionConfig based on the zkAddress. So we don't have 2 possibilities in the internal code implementations.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly merged pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
narendly merged pull request #1183:
URL: https://github.com/apache/helix/pull/1183
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r462459511
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerFactory.java
##########
@@ -65,4 +68,38 @@ public static HelixManager getZKHelixManager(String clusterName, String instance
return new ZKHelixManager(clusterName, instanceName, type, zkAddr, stateListener);
}
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given. If a proper
+ * ZkConnectionConfig. HelixManagerProperty given must contain a valid ZkConnectionConfig.
Review comment:
Thanks. Will fix!
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerFactory.java
##########
@@ -65,4 +68,38 @@ public static HelixManager getZKHelixManager(String clusterName, String instance
return new ZKHelixManager(clusterName, instanceName, type, zkAddr, stateListener);
}
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given. If a proper
+ * ZkConnectionConfig. HelixManagerProperty given must contain a valid ZkConnectionConfig.
+ * @param clusterName
+ * @param instanceName
+ * @param type
+ * @param stateListener
+ * @param helixManagerProperty must contain a valid ZkConnectionConfig
+ * @return
+ */
+ public static HelixManager getZKHelixManager(String clusterName, String instanceName,
+ InstanceType type, HelixManagerStateListener stateListener,
+ HelixManagerProperty helixManagerProperty) {
+ return new ZKHelixManager(clusterName, instanceName, type, null, stateListener,
+ helixManagerProperty);
+ }
+
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given. If a proper
+ * ZkConnectionConfig is given in HelixManagerProperty, zkAddr field will be overriden.
+ * @param clusterName
+ * @param instanceName
+ * @param type
+ * @param zkAddr will be overriden if a valid ZkConnectionConfig is given in helixManagerProperty
Review comment:
Let us add a check for validation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r465428282
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerFactory.java
##########
@@ -65,4 +64,39 @@ public static HelixManager getZKHelixManager(String clusterName, String instance
return new ZKHelixManager(clusterName, instanceName, type, zkAddr, stateListener);
}
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given.
+ * HelixManagerProperty given must contain a valid ZkConnectionConfig.
+ * @param clusterName
+ * @param instanceName
+ * @param type
+ * @param stateListener
+ * @param helixManagerProperty must contain a valid ZkConnectionConfig
+ * @return
+ */
+ public static HelixManager getZKHelixManager(String clusterName, String instanceName,
+ InstanceType type, HelixManagerStateListener stateListener,
+ HelixManagerProperty helixManagerProperty) {
+ return new ZKHelixManager(clusterName, instanceName, type, null, stateListener,
+ helixManagerProperty);
+ }
+
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given.
+ * NOTE: if both zkAddr and a valid ZkConnectionConfig are given in HelixManagerProperty, the
+ * instantiation will fail - only one is required.
+ * @param clusterName
+ * @param instanceName
+ * @param type
+ * @param zkAddr
+ * @param stateListener
+ * @param helixManagerProperty
+ * @return
+ */
+ public static HelixManager getZKHelixManager(String clusterName, String instanceName,
+ InstanceType type, String zkAddr, HelixManagerStateListener stateListener,
+ HelixManagerProperty helixManagerProperty) {
Review comment:
My thought is the same as what you described. We can always add it later if needed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r464515535
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -111,7 +111,7 @@
public static final int DEFAULT_MAX_DISCONNECT_THRESHOLD = 600; // Default to be a large number
private static final int DEFAULT_WAIT_CONNECTED_TIMEOUT = 10 * 1000; // wait until connected for up to 10 seconds.
- protected final String _zkAddress;
+ protected String _zkAddress;
Review comment:
Now that we've changed the design for HelixManager constructors to always accept zkAddress, we could bring the `final` keyword back.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r463228808
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerFactory.java
##########
@@ -65,4 +64,39 @@ public static HelixManager getZKHelixManager(String clusterName, String instance
return new ZKHelixManager(clusterName, instanceName, type, zkAddr, stateListener);
}
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given.
+ * HelixManagerProperty given must contain a valid ZkConnectionConfig.
+ * @param clusterName
+ * @param instanceName
+ * @param type
+ * @param stateListener
+ * @param helixManagerProperty must contain a valid ZkConnectionConfig
+ * @return
+ */
+ public static HelixManager getZKHelixManager(String clusterName, String instanceName,
+ InstanceType type, HelixManagerStateListener stateListener,
+ HelixManagerProperty helixManagerProperty) {
+ return new ZKHelixManager(clusterName, instanceName, type, null, stateListener,
+ helixManagerProperty);
+ }
+
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given.
+ * NOTE: if both zkAddr and a valid ZkConnectionConfig are given in HelixManagerProperty, the
+ * instantiation will fail - only one is required.
+ * @param clusterName
+ * @param instanceName
+ * @param type
+ * @param zkAddr
+ * @param stateListener
+ * @param helixManagerProperty
+ * @return
+ */
+ public static HelixManager getZKHelixManager(String clusterName, String instanceName,
+ InstanceType type, String zkAddr, HelixManagerStateListener stateListener,
+ HelixManagerProperty helixManagerProperty) {
Review comment:
As to whether we should "move" `zkAddress` into helixManagerProperty - this idea is not ideal. It is what we have now and we should keep it in the constructor for backward-compatibility, and giving users multiple ways to input `zkAddress` will undoubtedly confuse users.
So I think the right thing to do here is to keep zkAddress in HelixManager/HelixManagerFactory as a parameter. It won't continue to increase getZkHelixManager() methods. All future configs that are needed to generate an instance of HelixManager could go into `helixManagerProperty`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r463242944
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerProperty.java
##########
@@ -20,54 +20,82 @@
*/
import java.util.Properties;
+
import org.apache.helix.model.CloudConfig;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+
/**
- * Hold Helix manager properties. The manager properties further hold Helix cloud properties
- * and some other properties specific for the manager.
+ * HelixManagerProperty is a general property/config object used for HelixManager creation.
*/
public class HelixManagerProperty {
private static final Logger LOG = LoggerFactory.getLogger(HelixManagerProperty.class.getName());
private String _version;
private long _healthReportLatency;
private HelixCloudProperty _helixCloudProperty;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig _zkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig _zkClientConfig;
/**
+ * ** Deprecated - HelixManagerProperty should be a general property/config object used for
+ * HelixManager creation, not tied only to Properties or CloudConfig **
+ *
* Initialize Helix manager property with default value
* @param helixManagerProperties helix manager related properties input as a map
* @param cloudConfig cloudConfig read from Zookeeper
*/
+ @Deprecated
public HelixManagerProperty(Properties helixManagerProperties, CloudConfig cloudConfig) {
_helixCloudProperty = new HelixCloudProperty(cloudConfig);
setVersion(helixManagerProperties.getProperty(SystemPropertyKeys.HELIX_MANAGER_VERSION));
setHealthReportLatency(
helixManagerProperties.getProperty(SystemPropertyKeys.PARTICIPANT_HEALTH_REPORT_LATENCY));
}
+ public HelixManagerProperty() {
Review comment:
Update: the factory and builder are all serving different purposes and might be useful to have in the future. We decided to keep both.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r463202816
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -131,6 +131,7 @@
private int _reportLatency;
protected RealmAwareZkClient _zkclient;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig _realmAwareZkConnectionConfig;
Review comment:
As I pinged you in slack, if we can let RealmAwareZkConnectionConfig backward supports _zkAddress, then we can save tons of validations and branches code in this class.
Moreover, we can simplify the code following this principle,
1. new APIs only accept RealmAwareZkConnectionConfig, we don't use zkAddress directly.
2. for older API, we create default RealmAwareZkConnectionConfig based on the zkAddress. So we don't have 2 possibilities in the internal code implementations.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r464517443
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -1464,4 +1471,48 @@ private RealmAwareZkClient resolveZkClient(HelixZkClientFactory zkClientFactory,
private String buildShardingKey() {
return _clusterName.charAt(0) == '/' ? _clusterName : "/" + _clusterName;
}
+
+ /**
+ * 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.
+ * @param zkAddress
+ * @param helixManagerProperty
+ */
+ private void validateZkConnectionSettings(String zkAddress,
+ HelixManagerProperty helixManagerProperty) {
+ if (helixManagerProperty != null && helixManagerProperty.getZkConnectionConfig() != null) {
+ if (zkAddress != null && !zkAddress.isEmpty()) {
+ throw new HelixException(
+ "ZKHelixManager: cannot have both ZkAddress and ZkConnectionConfig set!");
+ }
+ RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
+ helixManagerProperty.getZkConnectionConfig();
+ 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;
+ }
+ }
+
+ /**
+ * Resolve ZK connection info for logging purposes.
+ * @return
+ */
+ private String getZkConnectionInfo() {
+ String zkConnectionInfo;
+ if (_zkAddress == null) {
+ if (_helixManagerProperty != null && _helixManagerProperty.getZkConnectionConfig() != null) {
+ zkConnectionInfo = _helixManagerProperty.getZkConnectionConfig().toString();
+ } else {
+ zkConnectionInfo = "ZkAddr and ZkConnectionConfig are null!";
Review comment:
Changed to "None".
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r464517074
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -1464,4 +1471,48 @@ private RealmAwareZkClient resolveZkClient(HelixZkClientFactory zkClientFactory,
private String buildShardingKey() {
return _clusterName.charAt(0) == '/' ? _clusterName : "/" + _clusterName;
}
+
+ /**
+ * 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.
+ * @param zkAddress
+ * @param helixManagerProperty
+ */
+ private void validateZkConnectionSettings(String zkAddress,
+ HelixManagerProperty helixManagerProperty) {
+ if (helixManagerProperty != null && helixManagerProperty.getZkConnectionConfig() != null) {
+ if (zkAddress != null && !zkAddress.isEmpty()) {
Review comment:
Sure, removing `&& !zkAddress.isEmpty()`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r465444581
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerFactory.java
##########
@@ -65,4 +64,39 @@ public static HelixManager getZKHelixManager(String clusterName, String instance
return new ZKHelixManager(clusterName, instanceName, type, zkAddr, stateListener);
}
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given.
+ * HelixManagerProperty given must contain a valid ZkConnectionConfig.
+ * @param clusterName
+ * @param instanceName
+ * @param type
+ * @param stateListener
+ * @param helixManagerProperty must contain a valid ZkConnectionConfig
+ * @return
+ */
+ public static HelixManager getZKHelixManager(String clusterName, String instanceName,
+ InstanceType type, HelixManagerStateListener stateListener,
+ HelixManagerProperty helixManagerProperty) {
+ return new ZKHelixManager(clusterName, instanceName, type, null, stateListener,
+ helixManagerProperty);
+ }
+
+ /**
+ * Construct a ZkHelixManager using the HelixManagerProperty instance given.
+ * NOTE: if both zkAddr and a valid ZkConnectionConfig are given in HelixManagerProperty, the
+ * instantiation will fail - only one is required.
+ * @param clusterName
+ * @param instanceName
+ * @param type
+ * @param zkAddr
+ * @param stateListener
+ * @param helixManagerProperty
+ * @return
+ */
+ public static HelixManager getZKHelixManager(String clusterName, String instanceName,
+ InstanceType type, String zkAddr, HelixManagerStateListener stateListener,
+ HelixManagerProperty helixManagerProperty) {
Review comment:
It seems this comment has been marked as resolved without finishing the code change. Are you still working on this 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on pull request #1183:
URL: https://github.com/apache/helix/pull/1183#issuecomment-673742067
This PR is ready to be merged
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r461970769
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerProperty.java
##########
@@ -20,54 +20,82 @@
*/
import java.util.Properties;
+
import org.apache.helix.model.CloudConfig;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+
/**
- * Hold Helix manager properties. The manager properties further hold Helix cloud properties
- * and some other properties specific for the manager.
+ * HelixManagerProperty is a general property/config object used for HelixManager creation.
*/
public class HelixManagerProperty {
private static final Logger LOG = LoggerFactory.getLogger(HelixManagerProperty.class.getName());
private String _version;
private long _healthReportLatency;
private HelixCloudProperty _helixCloudProperty;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig _zkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig _zkClientConfig;
/**
+ * ** Deprecated - HelixManagerProperty should be a general property/config object used for
+ * HelixManager creation, not tied only to Properties or CloudConfig **
+ *
* Initialize Helix manager property with default value
* @param helixManagerProperties helix manager related properties input as a map
* @param cloudConfig cloudConfig read from Zookeeper
*/
+ @Deprecated
Review comment:
I agree with this change.
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerProperty.java
##########
@@ -20,54 +20,82 @@
*/
import java.util.Properties;
+
import org.apache.helix.model.CloudConfig;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+
/**
- * Hold Helix manager properties. The manager properties further hold Helix cloud properties
- * and some other properties specific for the manager.
+ * HelixManagerProperty is a general property/config object used for HelixManager creation.
*/
public class HelixManagerProperty {
private static final Logger LOG = LoggerFactory.getLogger(HelixManagerProperty.class.getName());
private String _version;
private long _healthReportLatency;
private HelixCloudProperty _helixCloudProperty;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig _zkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig _zkClientConfig;
/**
+ * ** Deprecated - HelixManagerProperty should be a general property/config object used for
+ * HelixManager creation, not tied only to Properties or CloudConfig **
+ *
* Initialize Helix manager property with default value
* @param helixManagerProperties helix manager related properties input as a map
* @param cloudConfig cloudConfig read from Zookeeper
*/
+ @Deprecated
public HelixManagerProperty(Properties helixManagerProperties, CloudConfig cloudConfig) {
_helixCloudProperty = new HelixCloudProperty(cloudConfig);
setVersion(helixManagerProperties.getProperty(SystemPropertyKeys.HELIX_MANAGER_VERSION));
setHealthReportLatency(
helixManagerProperties.getProperty(SystemPropertyKeys.PARTICIPANT_HEALTH_REPORT_LATENCY));
}
+ public HelixManagerProperty() {
Review comment:
I think the plan is only allowing the property construct through HelixPropertyFactory. Shall we just make this constructor package-private? Also the other set methods.
@zhangmeng916 Please correct me if this is not the plan.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -225,9 +226,24 @@ public ZKHelixManager(String clusterName, String instanceName, InstanceType inst
String zkAddress, HelixManagerStateListener stateListener,
HelixManagerProperty helixManagerProperty) {
Review comment:
helixManagerProperty.RealmAwareZkConnectionConfig now contains zk connection information that possibly conflicts with zkAddress. This is dangerous.
Actually, I already see one potential bug in the method getMetadataStoreConnectionString. Which always returns the zkAddress only without checking the connectionConfig.
Please ensure we are only recording realmAwareZkConnectionConfig. Let's try to remove _zkAddress for good. Or at least update this field according to the realmAwareZkConnectionConfig input.
Other configurations such as sessionTimeout are in the same situation.
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManagerProperty.java
##########
@@ -20,54 +20,82 @@
*/
import java.util.Properties;
+
import org.apache.helix.model.CloudConfig;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+
/**
- * Hold Helix manager properties. The manager properties further hold Helix cloud properties
- * and some other properties specific for the manager.
+ * HelixManagerProperty is a general property/config object used for HelixManager creation.
*/
public class HelixManagerProperty {
private static final Logger LOG = LoggerFactory.getLogger(HelixManagerProperty.class.getName());
private String _version;
private long _healthReportLatency;
private HelixCloudProperty _helixCloudProperty;
+ private RealmAwareZkClient.RealmAwareZkConnectionConfig _zkConnectionConfig;
+ private RealmAwareZkClient.RealmAwareZkClientConfig _zkClientConfig;
/**
+ * ** Deprecated - HelixManagerProperty should be a general property/config object used for
+ * HelixManager creation, not tied only to Properties or CloudConfig **
+ *
* Initialize Helix manager property with default value
* @param helixManagerProperties helix manager related properties input as a map
* @param cloudConfig cloudConfig read from Zookeeper
*/
+ @Deprecated
public HelixManagerProperty(Properties helixManagerProperties, CloudConfig cloudConfig) {
_helixCloudProperty = new HelixCloudProperty(cloudConfig);
setVersion(helixManagerProperties.getProperty(SystemPropertyKeys.HELIX_MANAGER_VERSION));
setHealthReportLatency(
helixManagerProperties.getProperty(SystemPropertyKeys.PARTICIPANT_HEALTH_REPORT_LATENCY));
}
+ public HelixManagerProperty() {
Review comment:
So all the config read/processing work is in HelixPropertyFactory. In this case, we won't have unexpected overwrite or misusage of the property object. The flexibility should be in HelixPropertyFactory only.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -225,9 +226,24 @@ public ZKHelixManager(String clusterName, String instanceName, InstanceType inst
String zkAddress, HelixManagerStateListener stateListener,
HelixManagerProperty helixManagerProperty) {
Review comment:
Ideally, we should deprecate this one but create a constructor with clean parameters.
Besides a real concern is that if one passes a wrong or invalid zkAddress with this constructor (since he or she assumes it will be overridden by ZkConnectionConfig), then getMetadataStoreConnectionString will return this invalid value too. And the WAGED rebalancer won't work.
A slightly less serious issue would be, most of our logs are using zkAddress, so it would be harder for us to debug.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -225,9 +226,24 @@ public ZKHelixManager(String clusterName, String instanceName, InstanceType inst
String zkAddress, HelixManagerStateListener stateListener,
HelixManagerProperty helixManagerProperty) {
Review comment:
Ideally, we should deprecate this one but create a constructor with clean parameters list.
Besides a real concern is that if one passes a wrong or invalid zkAddress with this constructor (since he or she assumes it will be overridden by ZkConnectionConfig), then getMetadataStoreConnectionString will return this invalid value too. And the WAGED rebalancer won't work.
A slightly less serious issue would be, most of our logs are using zkAddress, so it would be harder for us to debug.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -225,9 +226,24 @@ public ZKHelixManager(String clusterName, String instanceName, InstanceType inst
String zkAddress, HelixManagerStateListener stateListener,
HelixManagerProperty helixManagerProperty) {
Review comment:
Ideally, we should deprecate this one but create a constructor with clean parameters list.
Besides, a real concern is that if one passes a wrong or invalid zkAddress with this constructor (since he or she assumes it will be overridden by ZkConnectionConfig), then getMetadataStoreConnectionString will return this invalid value too. And the WAGED rebalancer won't work.
A slightly less serious issue would be, most of our logs are using zkAddress, so it would be harder for us to debug.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -225,9 +226,24 @@ public ZKHelixManager(String clusterName, String instanceName, InstanceType inst
String zkAddress, HelixManagerStateListener stateListener,
HelixManagerProperty helixManagerProperty) {
Review comment:
Ideally, we should deprecate this one but create a constructor with clean parameters list.
Besides, a real concern is that if one passes a wrong or invalid zkAddress with this constructor (since he or she assumes it will be overridden by ZkConnectionConfig), then getMetadataStoreConnectionString will return this invalid value too. And the WAGED rebalancer won't work. So basically we don't have a good way of supporting WAGED when using multiple ZK Realm?
A slightly less serious issue would be, most of our logs are using zkAddress, so it would be harder for us to debug.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1183:
URL: https://github.com/apache/helix/pull/1183#discussion_r463224258
##########
File path: helix-core/src/main/java/org/apache/helix/HelixPropertyFactory.java
##########
@@ -88,8 +88,9 @@ private CloudConfig getCloudConfig(String zkAddress, String clusterName) {
CloudConfig cloudConfig;
RealmAwareZkClient dedicatedZkClient = null;
try {
- if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED)) {
- // If the multi ZK config is enabled, use multi-realm mode with DedicatedZkClient
+ if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED) || zkAddress == null) {
Review comment:
I have the comment explaining the behavior. Please suggest if you have another way to achieve this otherwise.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org