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