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