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 17:54:02 UTC
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1183: Add HelixManager constructor with RealmAwareZkConnectionConfig
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