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