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 2021/02/08 23:24:12 UTC
[GitHub] [helix] NealSun96 opened a new pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
NealSun96 opened a new pull request #1644:
URL: https://github.com/apache/helix/pull/1644
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1643
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
When not specifying a zkaddress or enabling multizk, it's possible for customers to not specify an msds endpoint in the system properties. When that's the case, the federated zkclient created in TaskStateModelFactory will fail. The msds endpoint contained in the manager's connection config should be respected.
### Tests
- The following is the result of the "mvn test" command on the appropriate module:
(If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r573355100
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -140,19 +139,18 @@ protected static RealmAwareZkClient createZkClient(HelixManager manager) {
// and some connect the manager before registering the state model factory (in which case we
// can use manager's connection). We need to think about the right order and determine if we
// want to enforce it, which may cause backward incompatibility.
+ if (!(manager instanceof ZKHelixManager)) {
Review comment:
Do we want to add a TODO here to indicate that we might need to extend the support in the future?
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManager.java
##########
@@ -50,6 +50,7 @@
import org.apache.helix.participant.StateMachineEngine;
import org.apache.helix.spectator.RoutingTableProvider;
import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
Review comment:
Remove
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r573117158
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -147,10 +147,16 @@ protected static RealmAwareZkClient createZkClient(HelixManager manager) {
if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED) || zkAddress == null) {
String clusterName = manager.getClusterName();
String shardingKey = HelixUtil.clusterNameToShardingKey(clusterName);
+ RealmAwareZkClient.RealmAwareZkConnectionConfig zkConnectionConfig =
Review comment:
You're right, we don't have to create a sharding key and rebuild config anymore. Changing.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on pull request #1644:
URL: https://github.com/apache/helix/pull/1644#issuecomment-777673025
This PR is ready to be merged, approved by @jiajunwang
Final commit message:
## Fix TaskStateModelFactory no msds endpoint multizk situation ##
Fixes a case where msds endpoint isn't provided in system property. This bug is caused by a timing issue where code in a feature branch isn't changed when another feature was introduced.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r574166793
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -140,19 +140,28 @@ protected static RealmAwareZkClient createZkClient(HelixManager manager) {
// and some connect the manager before registering the state model factory (in which case we
// can use manager's connection). We need to think about the right order and determine if we
// want to enforce it, which may cause backward incompatibility.
+ if (!(manager instanceof ZKHelixManager)) {
+ // TODO: None-ZKHelixManager cannot initialize this class. After interface rework of
+ // HelixManager, the initialization should be allowed.
+ throw new UnsupportedOperationException(
+ "Only ZKHelixManager is supported for configurable thread pool.");
+ }
RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
new RealmAwareZkClient.RealmAwareZkClientConfig().setZkSerializer(new ZNRecordSerializer());
String zkAddress = manager.getMetadataStoreConnectionString();
if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED) || zkAddress == null) {
- String clusterName = manager.getClusterName();
- String shardingKey = HelixUtil.clusterNameToShardingKey(clusterName);
- RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
- new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
- .setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
- .setZkRealmShardingKey(shardingKey).build();
+ RealmAwareZkClient.RealmAwareZkConnectionConfig zkConnectionConfig =
+ ((ZKHelixManager) manager).getRealmAwareZkConnectionConfig();
+ if (zkConnectionConfig == null) {
+ String clusterName = manager.getClusterName();
+ String shardingKey = HelixUtil.clusterNameToShardingKey(clusterName);
+ zkConnectionConfig = new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
+ .setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
+ .setZkRealmShardingKey(shardingKey).build();
+ }
Review comment:
How are we able to infer this from `zkConnectionConfig` being `null`? If the client creates a HelixManager to use it in a multi-zk environment, they should have provided a non-null `zkConnectionConfig` so we shouldn't expect it to be null, correct? If it is null, shouldn't we throw an exception instead?
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r572499359
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -147,10 +147,16 @@ protected static RealmAwareZkClient createZkClient(HelixManager manager) {
if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED) || zkAddress == null) {
Review comment:
This probably should be pointed out in https://github.com/apache/helix/pull/1627.
if zkAddress is null, we use federatedZkClient, which is not exactly the same as dedicated ZkClient.
My question is more like this:
Why do we think that MULTI_ZK_ENABLED and zkAddress == null belongs tot the same handling logic?
Or put it another way, this condition, "if zkAddress is null for the HelixManager passed to TaskStateModelFactory and multizk is not enabled"? what should be the expected behavior?
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r572475708
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManager.java
##########
@@ -362,6 +363,13 @@ void addExternalViewChangeListener(org.apache.helix.ExternalViewChangeListener l
*/
String getMetadataStoreConnectionString();
+ /**
+ * @return the RealmAwareZkConnectionConfig usd to create a realm aware zkClient
+ */
+ default RealmAwareZkClient.RealmAwareZkConnectionConfig getRealmAwareZkConnectionConfig() {
Review comment:
This implementation is a result of our (you and I mainly) discussion when Configurable Task Pool was first created, and I don't think the situation has changed.
See line 137 in TaskStateModelFactory.java where the TODO is. The main problem is that some customers register TaskStateModel before connecting the manager, some do it after. For those that register before connecting, we have to create a separate zkClient. If we enforce the ordering, customers have to change participant code, which we decided against.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r573305753
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -147,10 +147,16 @@ protected static RealmAwareZkClient createZkClient(HelixManager manager) {
if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED) || zkAddress == null) {
String clusterName = manager.getClusterName();
String shardingKey = HelixUtil.clusterNameToShardingKey(clusterName);
+ RealmAwareZkClient.RealmAwareZkConnectionConfig zkConnectionConfig =
+ manager.getRealmAwareZkConnectionConfig();
RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
.setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
- .setZkRealmShardingKey(shardingKey).build();
+ .setZkRealmShardingKey(shardingKey).setRoutingDataSourceEndpoint(
+ (zkConnectionConfig != null) ? zkConnectionConfig.getRoutingDataSourceEndpoint()
+ : null).setRoutingDataSourceType(
+ (zkConnectionConfig != null) ? zkConnectionConfig.getRoutingDataSourceType() : null)
+ .build();
Review comment:
Resolved offline with @jiajunwang: the design concern over ZooScalability is unrelated to this particular bug fix.
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r572503655
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -147,10 +147,16 @@ protected static RealmAwareZkClient createZkClient(HelixManager manager) {
if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED) || zkAddress == null) {
Review comment:
Let me rephrase it this way, under this situation, which ZooKeeper should we use?
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r574191766
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -140,19 +140,28 @@ protected static RealmAwareZkClient createZkClient(HelixManager manager) {
// and some connect the manager before registering the state model factory (in which case we
// can use manager's connection). We need to think about the right order and determine if we
// want to enforce it, which may cause backward incompatibility.
+ if (!(manager instanceof ZKHelixManager)) {
+ // TODO: None-ZKHelixManager cannot initialize this class. After interface rework of
+ // HelixManager, the initialization should be allowed.
+ throw new UnsupportedOperationException(
+ "Only ZKHelixManager is supported for configurable thread pool.");
+ }
RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
new RealmAwareZkClient.RealmAwareZkClientConfig().setZkSerializer(new ZNRecordSerializer());
String zkAddress = manager.getMetadataStoreConnectionString();
if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED) || zkAddress == null) {
- String clusterName = manager.getClusterName();
- String shardingKey = HelixUtil.clusterNameToShardingKey(clusterName);
- RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
- new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
- .setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
- .setZkRealmShardingKey(shardingKey).build();
+ RealmAwareZkClient.RealmAwareZkConnectionConfig zkConnectionConfig =
+ ((ZKHelixManager) manager).getRealmAwareZkConnectionConfig();
+ if (zkConnectionConfig == null) {
+ String clusterName = manager.getClusterName();
+ String shardingKey = HelixUtil.clusterNameToShardingKey(clusterName);
+ zkConnectionConfig = new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
+ .setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
+ .setZkRealmShardingKey(shardingKey).build();
+ }
Review comment:
Resolved offline with @narendly : this is necessary because, once again, no ordering is guaranteed between HelixManager.connect() and TaskStateModelFactory creation. It is entirely valid for a HelixManager to exist without zkConnectionConfig.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r572570119
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -147,10 +147,16 @@ protected static RealmAwareZkClient createZkClient(HelixManager manager) {
if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED) || zkAddress == null) {
String clusterName = manager.getClusterName();
String shardingKey = HelixUtil.clusterNameToShardingKey(clusterName);
+ RealmAwareZkClient.RealmAwareZkConnectionConfig zkConnectionConfig =
Review comment:
Could you explain why we don't just use zkConnectionConfig directly?
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang edited a comment on pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
jiajunwang edited a comment on pull request #1644:
URL: https://github.com/apache/helix/pull/1644#issuecomment-776235521
An alternative way that I think would be cleaner and future proof, we make the create ZkClient method in the HelixManager public. Of course, we may need to refine this method since it is for now a private method only. But as long as it is done, the HelixManager users no longer need to care about the internal details. The manager object just generates a new client according to the initial config.
This is a proposal I thought for a while. There are several benefits,
1. Hide details, as mentioned above.
2. Potentially simplifying code. We can even enforce all the Helix user to create client through a HelixManager in the future. So this class becomes the only one who understands the necessary mess.
3. Potentially a better place (or another handy layer) to manage connections compared with the other static pool object.
I suggest we discuss this, and maybe other ideas, before push forward this PR. Since if we choose a different direction, then we shall not add more methods to the public interface.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r572527927
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -147,10 +147,16 @@ protected static RealmAwareZkClient createZkClient(HelixManager manager) {
if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED) || zkAddress == null) {
Review comment:
Under this condition, like all other conditions modified in #1183, simply means we're in multiZk mode. The ZooKeeper used is the one corresponding to the cluster (which is made into a sharding key).
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r572529524
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -147,10 +147,16 @@ protected static RealmAwareZkClient createZkClient(HelixManager manager) {
if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED) || zkAddress == null) {
String clusterName = manager.getClusterName();
String shardingKey = HelixUtil.clusterNameToShardingKey(clusterName);
+ RealmAwareZkClient.RealmAwareZkConnectionConfig zkConnectionConfig =
+ manager.getRealmAwareZkConnectionConfig();
RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
.setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
- .setZkRealmShardingKey(shardingKey).build();
+ .setZkRealmShardingKey(shardingKey).setRoutingDataSourceEndpoint(
+ (zkConnectionConfig != null) ? zkConnectionConfig.getRoutingDataSourceEndpoint()
+ : null).setRoutingDataSourceType(
+ (zkConnectionConfig != null) ? zkConnectionConfig.getRoutingDataSourceType() : null)
+ .build();
Review comment:
Please see PR description for why this is done. We need the routing data source endpoint from manager's connection config, because there're use cases where it's not provided among system properties.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r572500629
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -147,10 +147,16 @@ protected static RealmAwareZkClient createZkClient(HelixManager manager) {
if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED) || zkAddress == null) {
Review comment:
Hi Kai, please see https://github.com/apache/helix/pull/1183 for details. In short, there are customer requirements for this type of usage.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r574121097
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -692,6 +692,13 @@ public String getMetadataStoreConnectionString() {
return _zkAddress;
}
+ /**
+ * @return the RealmAwareZkConnectionConfig usd to create a realm aware zkClient
Review comment:
Nit: Please fix the Javadoc
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r573376072
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -140,19 +139,18 @@ protected static RealmAwareZkClient createZkClient(HelixManager manager) {
// and some connect the manager before registering the state model factory (in which case we
// can use manager's connection). We need to think about the right order and determine if we
// want to enforce it, which may cause backward incompatibility.
+ if (!(manager instanceof ZKHelixManager)) {
Review comment:
You can throw an [UnsupportedOperationException](https://docs.oracle.com/javase/6/docs/api/java/lang/UnsupportedOperationException.html) here and let the user know that only ZkHelixManager is currently supported. If we have different implementations of HelixManager down the road, you could add the support for them.
If there are custom implementations that extend HelixManager, they still can't create a ZkClient whether you have this change or not, so it's not making things any worse. Also, HelixManager interface already assumes a ZK-based implementation, so trying to accommodate some non-ZK HelixManager implementation out there that may or may not exist in this PR is a moot point. This is a good point to consider if we want to introduce a set of new interfaces that will replace existing data access interfaces though.
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r572535992
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -147,10 +147,16 @@ protected static RealmAwareZkClient createZkClient(HelixManager manager) {
if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED) || zkAddress == null) {
String clusterName = manager.getClusterName();
String shardingKey = HelixUtil.clusterNameToShardingKey(clusterName);
+ RealmAwareZkClient.RealmAwareZkConnectionConfig zkConnectionConfig =
+ manager.getRealmAwareZkConnectionConfig();
RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
.setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
- .setZkRealmShardingKey(shardingKey).build();
+ .setZkRealmShardingKey(shardingKey).setRoutingDataSourceEndpoint(
+ (zkConnectionConfig != null) ? zkConnectionConfig.getRoutingDataSourceEndpoint()
+ : null).setRoutingDataSourceType(
+ (zkConnectionConfig != null) ? zkConnectionConfig.getRoutingDataSourceType() : null)
+ .build();
Review comment:
They why #1183 configAccessor does not have `setZkRealmShardingKey(shardingKey).setRoutingDataSourceEndpoint(...).setRoutingDataSourceType(...)`?
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r572567438
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManager.java
##########
@@ -362,6 +363,13 @@ void addExternalViewChangeListener(org.apache.helix.ExternalViewChangeListener l
*/
String getMetadataStoreConnectionString();
+ /**
+ * @return the RealmAwareZkConnectionConfig usd to create a realm aware zkClient
+ */
+ default RealmAwareZkClient.RealmAwareZkConnectionConfig getRealmAwareZkConnectionConfig() {
Review comment:
Thanks for explaining the context. Yes, there are implementations that do not connect the manager prior to registering their task. Can we add a TODO or a separate issue tracking this? Now that we have different modes of underlying ZkClient instantiation, we need to be more careful than ever about not creating multiple ZkClients, which could only add to the confusion for future developers.
For the extent of this PR, I'd be fine creating a one-off ZkClient using the connection config.
With that said, why does this need to be a default implementation? Nothing wrong with using the default interface, but in this case, the default behavior is not well-defined. I prefer not making this default. You might need to add a dummy implementation for MockHelixManager.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r573098589
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManager.java
##########
@@ -362,6 +363,13 @@ void addExternalViewChangeListener(org.apache.helix.ExternalViewChangeListener l
*/
String getMetadataStoreConnectionString();
+ /**
+ * @return the RealmAwareZkConnectionConfig usd to create a realm aware zkClient
+ */
+ default RealmAwareZkClient.RealmAwareZkConnectionConfig getRealmAwareZkConnectionConfig() {
Review comment:
A TODO has been added at the place where the zkClient is created, so I think we have that covered.
About the default behavior, none of the test-only HelixManagers have the `_helixManagerProperty` field, so even if I add implementations, they will be `return null`.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 edited a comment on pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
NealSun96 edited a comment on pull request #1644:
URL: https://github.com/apache/helix/pull/1644#issuecomment-777673025
**Please don't merge yet, testing against customer codebase.**
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r572455203
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManager.java
##########
@@ -362,6 +363,13 @@ void addExternalViewChangeListener(org.apache.helix.ExternalViewChangeListener l
*/
String getMetadataStoreConnectionString();
+ /**
+ * @return the RealmAwareZkConnectionConfig usd to create a realm aware zkClient
+ */
+ default RealmAwareZkClient.RealmAwareZkConnectionConfig getRealmAwareZkConnectionConfig() {
Review comment:
A high-level comment:
- The overarching principle here is that the underlying ZkClients in the given HelixManager and the ZkClient we are using in TaskStateModelFactory have to have the same connection config.
- Instead of passing ZkConnectionConfig that returns a null as the default implementation, can we just have them share the underlying ZkClient?
- This way, we can guarantee that whatever HelixManager sees is exactly the same as what we see in TaskStateModelFactory, and we do not have to worry about managing the lifecycles of two separate ZkClients.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang merged pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #1644:
URL: https://github.com/apache/helix/pull/1644
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 removed a comment on pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
NealSun96 removed a comment on pull request #1644:
URL: https://github.com/apache/helix/pull/1644#issuecomment-777673025
**Please don't merge yet, testing against customer codebase.**
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r572567438
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManager.java
##########
@@ -362,6 +363,13 @@ void addExternalViewChangeListener(org.apache.helix.ExternalViewChangeListener l
*/
String getMetadataStoreConnectionString();
+ /**
+ * @return the RealmAwareZkConnectionConfig usd to create a realm aware zkClient
+ */
+ default RealmAwareZkClient.RealmAwareZkConnectionConfig getRealmAwareZkConnectionConfig() {
Review comment:
Thanks for explaining the context. Yes, there are implementations that do not connect the manager prior to registering their task. Can we add a TODO or a separate issue tracking this? Now that we have different modes of underlying ZkClient instantiation, we need to be more careful than ever about not creating multiple ZkClients, which could only add to the confusion for future developers.
For the extent of this PR, I'd be fine creating a one-off ZkClient using the connection config.
With that said, why does this need to be a default implementation? Nothing wrong with using the default interface, but in this case, the default behavior is not well-defined. I prefer not making this default.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on pull request #1644:
URL: https://github.com/apache/helix/pull/1644#issuecomment-778354782
This PR is ready to be merged, approved by @jiajunwang
Final commit message:
## Fix TaskStateModelFactory no msds endpoint multizk situation ##
Fix TaskStateModelFactory creation when msds endpoint is not included in system property. This bug was introduced because when the feature was introduced to allow no msds endpoint in system property, the code in this PR was still in a feature branch and didn't get updated properly.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r573237143
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -147,10 +147,16 @@ protected static RealmAwareZkClient createZkClient(HelixManager manager) {
if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED) || zkAddress == null) {
String clusterName = manager.getClusterName();
String shardingKey = HelixUtil.clusterNameToShardingKey(clusterName);
+ RealmAwareZkClient.RealmAwareZkConnectionConfig zkConnectionConfig =
+ manager.getRealmAwareZkConnectionConfig();
RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
.setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
- .setZkRealmShardingKey(shardingKey).build();
+ .setZkRealmShardingKey(shardingKey).setRoutingDataSourceEndpoint(
+ (zkConnectionConfig != null) ? zkConnectionConfig.getRoutingDataSourceEndpoint()
+ : null).setRoutingDataSourceType(
+ (zkConnectionConfig != null) ? zkConnectionConfig.getRoutingDataSourceType() : null)
+ .build();
Review comment:
I think the question is that what is the possible failure if we don't guard. But if you are considering if people will use it in the "wrong" way, then my comment is that when there is a way, someone will use it. And the bug will eventually be returned to us.
So as long as there is a possible hard failure case, please guard it. If it turns to be too complicated, then let's reconsider the overall design instead of patching here and there.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r573376072
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -140,19 +139,18 @@ protected static RealmAwareZkClient createZkClient(HelixManager manager) {
// and some connect the manager before registering the state model factory (in which case we
// can use manager's connection). We need to think about the right order and determine if we
// want to enforce it, which may cause backward incompatibility.
+ if (!(manager instanceof ZKHelixManager)) {
Review comment:
You can throw an [UnsupportedOperationException](https://docs.oracle.com/javase/6/docs/api/java/lang/UnsupportedOperationException.html) here and let the user know that only ZkHelixManager is currently supported. If we have different implementations of HelixManager down the road, you could add the support for them.
If there are custom implementations that extend HelixManager, they still can't create a ZkClient whether you have this change or not, so it's not making things any worse. Also, the HelixManager interface already assumes a ZK-based implementation, so trying to accommodate some non-ZK HelixManager implementation out there that may or may not exist in this PR is a moot point. This is a good point to consider if we want to introduce a set of new interfaces that will replace existing data access interfaces though.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r573235253
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManager.java
##########
@@ -362,6 +363,13 @@ void addExternalViewChangeListener(org.apache.helix.ExternalViewChangeListener l
*/
String getMetadataStoreConnectionString();
+ /**
+ * @return the RealmAwareZkConnectionConfig usd to create a realm aware zkClient
+ */
+ default RealmAwareZkClient.RealmAwareZkConnectionConfig getRealmAwareZkConnectionConfig() {
Review comment:
I think we should not even add the method to the HelixManager interface. 1. it is ZK config, conceptually, HelixManager does not have to be implemented by a ZkHelixManager. 2. if we expose more internal implementation details from the public interface, we may be stuck with this design and cannot change anything in the future. This is not a good OO design.
Please see my proposal that I put here earlier.
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1644:
URL: https://github.com/apache/helix/pull/1644#issuecomment-776235521
An alternative way that I think would be cleaner and future proof, we make the create ZkClient method in the HelixManager public. Of course, we may need to refine this method since it is for now a private method only. But as long as it is done, the HelixManager users no longer need to care about the internal details. The manager object just generates a new client according to the initial config.
This is a proposal I thought for a while. There are several benefits,
1. Hide details, as mentioned above.
2. Potentially simplifying code. We can even enforce all the Helix user to create client through a HelixManager in the future. So this class becomes the only one who understands the necessary mess.
3. Potentially a better place (or another handy layer) to manage connections compared with the other static pool object.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r573949927
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -140,19 +139,18 @@ protected static RealmAwareZkClient createZkClient(HelixManager manager) {
// and some connect the manager before registering the state model factory (in which case we
// can use manager's connection). We need to think about the right order and determine if we
// want to enforce it, which may cause backward incompatibility.
+ if (!(manager instanceof ZKHelixManager)) {
Review comment:
I'll add a TODO here. The fact that non-ZK HelixManager simply cannot create this factory is pretty absurd, especially given that we have a default value that can be used.
UnsupportedOperationException is a good suggestion, changing.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r573103943
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -147,10 +147,16 @@ protected static RealmAwareZkClient createZkClient(HelixManager manager) {
if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED) || zkAddress == null) {
String clusterName = manager.getClusterName();
String shardingKey = HelixUtil.clusterNameToShardingKey(clusterName);
+ RealmAwareZkClient.RealmAwareZkConnectionConfig zkConnectionConfig =
+ manager.getRealmAwareZkConnectionConfig();
RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
.setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
- .setZkRealmShardingKey(shardingKey).build();
+ .setZkRealmShardingKey(shardingKey).setRoutingDataSourceEndpoint(
+ (zkConnectionConfig != null) ? zkConnectionConfig.getRoutingDataSourceEndpoint()
+ : null).setRoutingDataSourceType(
+ (zkConnectionConfig != null) ? zkConnectionConfig.getRoutingDataSourceType() : null)
+ .build();
Review comment:
Some of them (like ConfigAccessor) seem to be deprecated and for testing only, but some of them are not. I think this is a good question for @narendly : is it necessary to guard the other places with similar patterns, such as `ZkBucketDataAccessor`?
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on pull request #1644:
URL: https://github.com/apache/helix/pull/1644#issuecomment-776299275
> An alternative way that I think would be cleaner and future proof, we make the create ZkClient method in the HelixManager public. Of course, we may need to refine this method since it is for now a private method only. But as long as it is done, the HelixManager users no longer need to care about the internal details. The manager object just generates a new client according to the initial config.
>
> This is a proposal I thought for a while. There are several benefits,
>
> 1. Hide details, as mentioned above.
> 2. Potentially simplifying code. We can even enforce all the Helix user to create client through a HelixManager in the future. So this class becomes the only one who understands the necessary mess.
> 3. Potentially a better place (or another handy layer) to manage connections compared with the other static pool object.
>
> I suggest we discuss this, and maybe other ideas, before push forward this PR. Since if we choose a different direction, then we shall not add more methods to the public interface.
Resolved with @jiajunwang offline, the design concern over ZooScalability is unrelated to this particular bug fix. We established that the difficulty of this code lies within the fact that TaskStateModelFactory may be initialized before ZkHelixManager connection, which is why we need to create a new ZkClient. @jiajunwang 's main concern is over the OO design, to which we have a solution now.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r572567438
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManager.java
##########
@@ -362,6 +363,13 @@ void addExternalViewChangeListener(org.apache.helix.ExternalViewChangeListener l
*/
String getMetadataStoreConnectionString();
+ /**
+ * @return the RealmAwareZkConnectionConfig usd to create a realm aware zkClient
+ */
+ default RealmAwareZkClient.RealmAwareZkConnectionConfig getRealmAwareZkConnectionConfig() {
Review comment:
Thanks for explaining the context. Yes, there are implementations that may not connect the manager prior to registering their task. Can we add a TODO or a separate issue tracking this? Now that we have different modes of underlying ZkClient instantiation, we need to be more careful about not creating multiple ZkClients, which could only add to the confusion for future developers.
For the extent of this PR, I'd be fine creating a one-off ZkClient using the connection config.
With that said, why does this need to be a default implementation? Nothing wrong with using the default interface, but in this case, the default behavior is not well-defined. I prefer not making this default.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r573305464
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManager.java
##########
@@ -362,6 +363,13 @@ void addExternalViewChangeListener(org.apache.helix.ExternalViewChangeListener l
*/
String getMetadataStoreConnectionString();
+ /**
+ * @return the RealmAwareZkConnectionConfig usd to create a realm aware zkClient
+ */
+ default RealmAwareZkClient.RealmAwareZkConnectionConfig getRealmAwareZkConnectionConfig() {
Review comment:
Resolved with @jiajunwang offline: not a good OO design, so better solution: moving this to `ZkHelixManager` only.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r573359292
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -140,19 +139,18 @@ protected static RealmAwareZkClient createZkClient(HelixManager manager) {
// and some connect the manager before registering the state model factory (in which case we
// can use manager's connection). We need to think about the right order and determine if we
// want to enforce it, which may cause backward incompatibility.
+ if (!(manager instanceof ZKHelixManager)) {
Review comment:
I actually want to get an opinion on this: is it really better to reject non-ZKHelixManagers, comparing to returning a null here, and let the util function to use the default pool size? This means open source customers who extend HelixManager literally cannot create a TaskStateModelFactory.
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r572505310
##########
File path: helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java
##########
@@ -147,10 +147,16 @@ protected static RealmAwareZkClient createZkClient(HelixManager manager) {
if (Boolean.getBoolean(SystemPropertyKeys.MULTI_ZK_ENABLED) || zkAddress == null) {
String clusterName = manager.getClusterName();
String shardingKey = HelixUtil.clusterNameToShardingKey(clusterName);
+ RealmAwareZkClient.RealmAwareZkConnectionConfig zkConnectionConfig =
+ manager.getRealmAwareZkConnectionConfig();
RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig =
new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder()
.setRealmMode(RealmAwareZkClient.RealmMode.SINGLE_REALM)
- .setZkRealmShardingKey(shardingKey).build();
+ .setZkRealmShardingKey(shardingKey).setRoutingDataSourceEndpoint(
+ (zkConnectionConfig != null) ? zkConnectionConfig.getRoutingDataSourceEndpoint()
+ : null).setRoutingDataSourceType(
+ (zkConnectionConfig != null) ? zkConnectionConfig.getRoutingDataSourceType() : null)
+ .build();
Review comment:
How do we reason `setZkRealmShardingKey(shardingKey).setRoutingDataSourceEndpoint(...).setRoutingDataSourceType(...)`?
Note, this pattern is different from #1183 `configAccessor`.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #1644: Fix TaskStateModelFactory no msds endpoint multizk situation
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1644:
URL: https://github.com/apache/helix/pull/1644#discussion_r572567438
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManager.java
##########
@@ -362,6 +363,13 @@ void addExternalViewChangeListener(org.apache.helix.ExternalViewChangeListener l
*/
String getMetadataStoreConnectionString();
+ /**
+ * @return the RealmAwareZkConnectionConfig usd to create a realm aware zkClient
+ */
+ default RealmAwareZkClient.RealmAwareZkConnectionConfig getRealmAwareZkConnectionConfig() {
Review comment:
Thanks for explaining the context. Yes, there are implementations that do not connect the manager prior to registering their task. Can we add a TODO or a separate issue tracking this? Now that we have different modes of underlying ZkClient instantiation, we need to be more careful about not creating multiple ZkClients, which could only add to the confusion for future developers.
For the extent of this PR, I'd be fine creating a one-off ZkClient using the connection config.
With that said, why does this need to be a default implementation? Nothing wrong with using the default interface, but in this case, the default behavior is not well-defined. I prefer not making this default.
----------------------------------------------------------------
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