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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
