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 2020/05/27 22:36:52 UTC
[GitHub] [helix] zhangmeng916 opened a new pull request #1033: Add path existing check for customized state
zhangmeng916 opened a new pull request #1033:
URL: https://github.com/apache/helix/pull/1033
### Issues
- [ X] My PR addresses the following Helix issues and references them in the PR description:
(#1032 )
### Description
- [X] Here are some details about my PR, including screenshots of any UI changes:
This PR adds check for the existence of customized state path before adding listener to it. The check is mainly for backward compatibility, as our customers may still use previous helix, and do not have customized state root path created. In that case, we should not add listener to non existing path.
### Tests
- [X ] The following is the result of the "mvn test" command on the appropriate module:
helix-core:
[ERROR] Tests run: 1147, Failures: 1, Errors: 0, Skipped: 1, Time elapsed: 4,717.53 s <<< FAILURE! - in TestSuite
[ERROR] testDisableCluster(org.apache.helix.integration.rebalancer.TestClusterInMaintenanceModeWhenReachingMaxPartition) Time elapsed: 5.078 s <<< FAILURE!
java.lang.AssertionError: expected:<true> but was:<false>
at org.apache.helix.integration.rebalancer.TestClusterInMaintenanceModeWhenReachingMaxPartition.testDisableCluster(TestClusterInMaintenanceModeWhenReachingMaxPartition.java:119)
[INFO]
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] TestClusterInMaintenanceModeWhenReachingMaxPartition.testDisableCluster:119 expected:<true> but was:<false>
[INFO]
[ERROR] Tests run: 1147, Failures: 1, Errors: 0, Skipped: 1
[INFO]
helix-rest:
[INFO] Tests run: 160, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 180.82 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 160, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 03:04 min
[INFO] Finished at: 2020-05-27T14:11:30-07:00
### Commits
- [X] 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"
### Documentation (Optional)
- [X] In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Code Quality
- [X] 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] zhangmeng916 commented on a change in pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r431505459
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -591,8 +591,11 @@ public void addCurrentStateChangeListener(org.apache.helix.CurrentStateChangeLis
@Override
public void addCustomizedStateRootChangeListener(CustomizedStateRootChangeListener listener,
String instanceName) throws Exception {
- addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
- ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ PropertyKey propertyKey = _dataAccessor.keyBuilder().customizedStatesRoot(instanceName);
+ if (_zkclient.exists(propertyKey.getPath())) {
Review comment:
That would impact all the exceptions right? Then it's not good for debugging purpose.
----------------------------------------------------------------
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 #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r431483789
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1256,14 +1256,18 @@ protected void checkLiveInstancesObservation(List<LiveInstance> liveInstances,
for (String instance : curInstances.keySet()) {
if (lastInstances == null || !lastInstances.containsKey(instance)) {
- try {
- manager.addCustomizedStateRootChangeListener(this, instance);
- logger.info(manager.getInstanceName() + " added customized root change listener for"
- + " " + instance
- + ", listener: " + this);
- } catch (Exception e) {
- logger.error("Fail to add customized root change listener for instance: " + instance,
- e);
+ PropertyKey propertyKey =
+ keyBuilder.customizedStatesRoot(instance);
+ if (propertyKey.getPath() != null) {
Review comment:
You're creating a PropertyKey object with the given instance name and checking whether it's null? How would it be null if you've just created the path? In other words, when would this condition ever evaluate to false?
Did you mean to check whether the path exists in ZooKeeper? If so, shouldn't you do an exist() check with a ZkClient-based API?
Unless I'm mistaken and we actually somehow pre-populate the KeyBuilder with a whitelist of instances for the CustomizedView feature?
----------------------------------------------------------------
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 #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r434866114
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1258,12 +1258,12 @@ protected void checkLiveInstancesObservation(List<LiveInstance> liveInstances,
if (lastInstances == null || !lastInstances.containsKey(instance)) {
try {
manager.addCustomizedStateRootChangeListener(this, instance);
- logger.info(manager.getInstanceName() + " added customized root change listener for"
- + " " + instance
- + ", listener: " + this);
+ logger.info(manager.getInstanceName() + " added root path listener for customized "
+ + "state change for" + " " + instance + ", listener: " + this);
} catch (Exception e) {
- logger.error("Fail to add customized root change listener for instance: " + instance,
- e);
+ logger.error(
+ "Fail to add root path listener for customized state change" + "for instance: "
Review comment:
Why use 2 separate sections for the constant string contents?
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1258,12 +1258,12 @@ protected void checkLiveInstancesObservation(List<LiveInstance> liveInstances,
if (lastInstances == null || !lastInstances.containsKey(instance)) {
try {
manager.addCustomizedStateRootChangeListener(this, instance);
- logger.info(manager.getInstanceName() + " added customized root change listener for"
- + " " + instance
- + ", listener: " + this);
+ logger.info(manager.getInstanceName() + " added root path listener for customized "
Review comment:
`+ "state change for" + " "`
Can we please refine the format as well.
----------------------------------------------------------------
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 #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r434965873
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -639,14 +639,20 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
}
}
} catch (ZkNoNodeException | HelixMetaDataAccessException e) {
- logger.warn(
- "fail to subscribe child/data change. path: " + path + ", listener: " + _listener, e);
+ // TODO: avoid calling getChildren for path that does not exist
+ if (_changeType == CUSTOMIZED_STATE_ROOT) {
+ logger.warn("Failed to subscribe child/data change. path: {}, listener: {}", path,
+ _listener);
+ } else {
+ logger.warn("Failed to subscribe child/data change. path: {}, listener: {}", path,
+ _listener, e);
Review comment:
Can we just merge these two conditions? There's practically no difference.
----------------------------------------------------------------
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] pkuwm commented on a change in pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r431503090
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -591,8 +591,11 @@ public void addCurrentStateChangeListener(org.apache.helix.CurrentStateChangeLis
@Override
public void addCustomizedStateRootChangeListener(CustomizedStateRootChangeListener listener,
String instanceName) throws Exception {
- addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
- ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ PropertyKey propertyKey = _dataAccessor.keyBuilder().customizedStatesRoot(instanceName);
+ if (_zkclient.exists(propertyKey.getPath())) {
Review comment:
Maybe we could just change the the log in callback handler not to log the exception stack trace.
----------------------------------------------------------------
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] zhangmeng916 commented on pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on pull request #1033:
URL: https://github.com/apache/helix/pull/1033#issuecomment-639629938
This PR is ready to merge. Approved by @pkuwm
Final commit message:
Remove stack trace when customized state root does not exist and throws no node exception.
Change logging in Callback handler to be parameterized logging.
----------------------------------------------------------------
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 #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r431497267
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -591,8 +591,11 @@ public void addCurrentStateChangeListener(org.apache.helix.CurrentStateChangeLis
@Override
public void addCustomizedStateRootChangeListener(CustomizedStateRootChangeListener listener,
String instanceName) throws Exception {
- addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
- ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ PropertyKey propertyKey = _dataAccessor.keyBuilder().customizedStatesRoot(instanceName);
+ if (_zkclient.exists(propertyKey.getPath())) {
+ addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
+ ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ }
Review comment:
Do you think it would be useful to log (warn) if this evaluates to false?
----------------------------------------------------------------
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] lei-xia commented on a change in pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
lei-xia commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r431975993
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -591,8 +591,11 @@ public void addCurrentStateChangeListener(org.apache.helix.CurrentStateChangeLis
@Override
public void addCustomizedStateRootChangeListener(CustomizedStateRootChangeListener listener,
String instanceName) throws Exception {
- addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
- ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ PropertyKey propertyKey = _dataAccessor.keyBuilder().customizedStatesRoot(instanceName);
+ if (_zkclient.exists(propertyKey.getPath())) {
Review comment:
Dummy question, why participant needs to listen on customized view changes? Only controller needs to listen on the change, not?
----------------------------------------------------------------
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] mgao0 commented on a change in pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r431995256
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -591,8 +591,11 @@ public void addCurrentStateChangeListener(org.apache.helix.CurrentStateChangeLis
@Override
public void addCustomizedStateRootChangeListener(CustomizedStateRootChangeListener listener,
String instanceName) throws Exception {
- addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
- ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ PropertyKey propertyKey = _dataAccessor.keyBuilder().customizedStatesRoot(instanceName);
+ if (_zkclient.exists(propertyKey.getPath())) {
+ addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
+ ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ }
Review comment:
I remember we say that if participant's version is older than the controller version, it's fine? If so then I think we may not need to log it.
----------------------------------------------------------------
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] zhangmeng916 commented on a change in pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r431978817
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -591,8 +591,11 @@ public void addCurrentStateChangeListener(org.apache.helix.CurrentStateChangeLis
@Override
public void addCustomizedStateRootChangeListener(CustomizedStateRootChangeListener listener,
String instanceName) throws Exception {
- addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
- ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ PropertyKey propertyKey = _dataAccessor.keyBuilder().customizedStatesRoot(instanceName);
+ if (_zkclient.exists(propertyKey.getPath())) {
Review comment:
Participant does not need to listen on customized view change. The backward compatibility issue happens when we have a new version controller, while the client is on an old version participant. The new path INSTANCES/CUSTOMIZED_STATES does not exist, but controller will try to add listener to that path during live instance change. Then zkclient will have a no node exception. It's fine, just a warn, but kind of pollute client's log and make them confused.
----------------------------------------------------------------
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 #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r431493134
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1256,14 +1256,18 @@ protected void checkLiveInstancesObservation(List<LiveInstance> liveInstances,
for (String instance : curInstances.keySet()) {
if (lastInstances == null || !lastInstances.containsKey(instance)) {
- try {
- manager.addCustomizedStateRootChangeListener(this, instance);
- logger.info(manager.getInstanceName() + " added customized root change listener for"
- + " " + instance
- + ", listener: " + this);
- } catch (Exception e) {
- logger.error("Fail to add customized root change listener for instance: " + instance,
- e);
+ PropertyKey propertyKey =
+ keyBuilder.customizedStatesRoot(instance);
+ if (propertyKey.getPath() != null) {
Review comment:
Yes, for backward compatibility, you need to check the root path existence in ZK. This check is not very meaningful. You're basically doing `if ("~~/instanceName" != null)`, which always evaluates to true.
You can use the data accessor from the HelixManager instance here for an exist check.
----------------------------------------------------------------
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] zhangmeng916 commented on a change in pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r431494734
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1256,14 +1256,18 @@ protected void checkLiveInstancesObservation(List<LiveInstance> liveInstances,
for (String instance : curInstances.keySet()) {
if (lastInstances == null || !lastInstances.containsKey(instance)) {
- try {
- manager.addCustomizedStateRootChangeListener(this, instance);
- logger.info(manager.getInstanceName() + " added customized root change listener for"
- + " " + instance
- + ", listener: " + this);
- } catch (Exception e) {
- logger.error("Fail to add customized root change listener for instance: " + instance,
- e);
+ PropertyKey propertyKey =
+ keyBuilder.customizedStatesRoot(instance);
+ if (propertyKey.getPath() != null) {
+ try {
+ manager.addCustomizedStateRootChangeListener(this, instance);
+ logger.info(
+ manager.getInstanceName() + " added customized root change listener for" + " " + instance
+ + ", listener: " + this);
+ } catch (Exception e) {
+ logger.error("Fail to add customized root change listener for instance: " + instance,
Review comment:
I updated the log information. For log.info or log.debug, in the controller, all other listeners are added with log.info. I think this is for easy debug purpose. Please let me know if the previous convention should be changed.
----------------------------------------------------------------
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 #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r431493134
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1256,14 +1256,18 @@ protected void checkLiveInstancesObservation(List<LiveInstance> liveInstances,
for (String instance : curInstances.keySet()) {
if (lastInstances == null || !lastInstances.containsKey(instance)) {
- try {
- manager.addCustomizedStateRootChangeListener(this, instance);
- logger.info(manager.getInstanceName() + " added customized root change listener for"
- + " " + instance
- + ", listener: " + this);
- } catch (Exception e) {
- logger.error("Fail to add customized root change listener for instance: " + instance,
- e);
+ PropertyKey propertyKey =
+ keyBuilder.customizedStatesRoot(instance);
+ if (propertyKey.getPath() != null) {
Review comment:
Yes, for backward compatibility, you need to check the root path existence in ZK. This check is not very meaningful. You're basically doing `if ("~~/instanceName" != 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] pkuwm commented on a change in pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r431501483
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -591,8 +591,11 @@ public void addCurrentStateChangeListener(org.apache.helix.CurrentStateChangeLis
@Override
public void addCustomizedStateRootChangeListener(CustomizedStateRootChangeListener listener,
String instanceName) throws Exception {
- addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
- ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ PropertyKey propertyKey = _dataAccessor.keyBuilder().customizedStatesRoot(instanceName);
+ if (_zkclient.exists(propertyKey.getPath())) {
Review comment:
I am concerned about the performance. This check adds more time for adding listeners because of network latency. I wonder if there is a better way so this check could be avoided.
----------------------------------------------------------------
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 #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r431541207
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -591,8 +591,11 @@ public void addCurrentStateChangeListener(org.apache.helix.CurrentStateChangeLis
@Override
public void addCustomizedStateRootChangeListener(CustomizedStateRootChangeListener listener,
String instanceName) throws Exception {
- addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
- ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ PropertyKey propertyKey = _dataAccessor.keyBuilder().customizedStatesRoot(instanceName);
+ if (_zkclient.exists(propertyKey.getPath())) {
Review comment:
exists() check is not a heavyweight ZK operation and considering we only call this when there's a LiveInstance change (or periodic rebalance), I don't think performance is a concern here.
----------------------------------------------------------------
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 #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r431493134
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1256,14 +1256,18 @@ protected void checkLiveInstancesObservation(List<LiveInstance> liveInstances,
for (String instance : curInstances.keySet()) {
if (lastInstances == null || !lastInstances.containsKey(instance)) {
- try {
- manager.addCustomizedStateRootChangeListener(this, instance);
- logger.info(manager.getInstanceName() + " added customized root change listener for"
- + " " + instance
- + ", listener: " + this);
- } catch (Exception e) {
- logger.error("Fail to add customized root change listener for instance: " + instance,
- e);
+ PropertyKey propertyKey =
+ keyBuilder.customizedStatesRoot(instance);
+ if (propertyKey.getPath() != null) {
Review comment:
Yes, for backward compatibility, you need to check the root path existence in ZK. This check is not very meaningful. You're basically doing `if ("~~/instanceName" != null)`, which always evaluates to true.
----------------------------------------------------------------
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] mgao0 commented on a change in pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r431995256
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -591,8 +591,11 @@ public void addCurrentStateChangeListener(org.apache.helix.CurrentStateChangeLis
@Override
public void addCustomizedStateRootChangeListener(CustomizedStateRootChangeListener listener,
String instanceName) throws Exception {
- addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
- ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ PropertyKey propertyKey = _dataAccessor.keyBuilder().customizedStatesRoot(instanceName);
+ if (_zkclient.exists(propertyKey.getPath())) {
+ addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
+ ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ }
Review comment:
Did we say that if participant's version is older than the controller version, it's fine? If so then I think we may not need to log it.
----------------------------------------------------------------
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 #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r434965873
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -639,14 +639,20 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
}
}
} catch (ZkNoNodeException | HelixMetaDataAccessException e) {
- logger.warn(
- "fail to subscribe child/data change. path: " + path + ", listener: " + _listener, e);
+ // TODO: avoid calling getChildren for path that does not exist
+ if (_changeType == CUSTOMIZED_STATE_ROOT) {
+ logger.warn("Failed to subscribe child/data change. path: {}, listener: {}", path,
+ _listener);
+ } else {
+ logger.warn("Failed to subscribe child/data change. path: {}, listener: {}", path,
+ _listener, e);
Review comment:
Can we just merge these two conditions? There's practically no difference. Or catch exceptions separately and log an appropriate, differentiated log message. Otherwise, this just looks like duplicate code.
----------------------------------------------------------------
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 #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r431483789
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1256,14 +1256,18 @@ protected void checkLiveInstancesObservation(List<LiveInstance> liveInstances,
for (String instance : curInstances.keySet()) {
if (lastInstances == null || !lastInstances.containsKey(instance)) {
- try {
- manager.addCustomizedStateRootChangeListener(this, instance);
- logger.info(manager.getInstanceName() + " added customized root change listener for"
- + " " + instance
- + ", listener: " + this);
- } catch (Exception e) {
- logger.error("Fail to add customized root change listener for instance: " + instance,
- e);
+ PropertyKey propertyKey =
+ keyBuilder.customizedStatesRoot(instance);
+ if (propertyKey.getPath() != null) {
Review comment:
You're creating a PropertyKey object with the given instance name and checking whether it's null? How would it be null if you've just created the path?
Did you mean to check whether the path exists in ZooKeeper? If so, shouldn't you do an exist() check with a ZkClient-based API?
Unless I'm mistaken and we actually somehow pre-populate the KeyBuilder with a whitelist of instances for the CustomizedView feature?
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1256,14 +1256,18 @@ protected void checkLiveInstancesObservation(List<LiveInstance> liveInstances,
for (String instance : curInstances.keySet()) {
if (lastInstances == null || !lastInstances.containsKey(instance)) {
- try {
- manager.addCustomizedStateRootChangeListener(this, instance);
- logger.info(manager.getInstanceName() + " added customized root change listener for"
- + " " + instance
- + ", listener: " + this);
- } catch (Exception e) {
- logger.error("Fail to add customized root change listener for instance: " + instance,
- e);
+ PropertyKey propertyKey =
+ keyBuilder.customizedStatesRoot(instance);
+ if (propertyKey.getPath() != null) {
+ try {
+ manager.addCustomizedStateRootChangeListener(this, instance);
+ logger.info(
+ manager.getInstanceName() + " added customized root change listener for" + " " + instance
+ + ", listener: " + this);
+ } catch (Exception e) {
+ logger.error("Fail to add customized root change listener for instance: " + instance,
Review comment:
"customized root change" is confusing. Could we explicitly state what it is? For example,
"Added (or Failed to add) the root path for Customized State ..."
Also, this log doesn't seem entirely necessary. Don't need to log every time we add a listener - I'd make this a debug log so as not to pollute the log.
----------------------------------------------------------------
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] zhangmeng916 commented on a change in pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r431487283
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1256,14 +1256,18 @@ protected void checkLiveInstancesObservation(List<LiveInstance> liveInstances,
for (String instance : curInstances.keySet()) {
if (lastInstances == null || !lastInstances.containsKey(instance)) {
- try {
- manager.addCustomizedStateRootChangeListener(this, instance);
- logger.info(manager.getInstanceName() + " added customized root change listener for"
- + " " + instance
- + ", listener: " + this);
- } catch (Exception e) {
- logger.error("Fail to add customized root change listener for instance: " + instance,
- e);
+ PropertyKey propertyKey =
+ keyBuilder.customizedStatesRoot(instance);
+ if (propertyKey.getPath() != null) {
Review comment:
oh..I was thinking to use exist checking, but put the wrong check, let me update it.
This is just for backward incompatibility check. We're totally fine without this check, but the participant would need to be on the latest version to have customized state path created. If they don't, there can be some error logs when adding listener to customized state path. It's still fine, but might cause confusion.
----------------------------------------------------------------
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 edited a comment on pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
narendly edited a comment on pull request #1033:
URL: https://github.com/apache/helix/pull/1033#issuecomment-635553574
Here's the copy of an offline discussion
> Jiajun Wang:man-woman-girl-girl: 12:27
> Meng will confirm. But I think that scenario was discussed and address in the original design.
> 12:28
> I will suggest keeping the root path listener (default feature for the new version controller) but the children's paths listeners will depend on the configuration.
>
>
>
>
>
> 12:29
> that is the minimum code change and logic is the simpliest.
> 12:30
> Basically, we only need to avoid the default subscription to children nodes in the root path subscribe process (edited)
>
> Hunter Lee 12:30
> So are we comfortable with creating CallbackHandlers for nonexistent root paths?
>
> Jiajun Wang 12:30
> Then all the rest parts have been handled by the current code.
> New
> 12:32
> That's the only downside. But my argument is that:
> It is a default feature for the new controller.
> It won't cause watcher leakage. It is always associated with a live callback handler, although never triggered,
> The new participant will always have this folder. That's the agreement.
> 12:33
> The alternative design will create many additional codes for the "perfect" backward compatibility.
> 12:33
> Which is not necessary IMO
>
> Hunter Lee 12:35
> Ok. Although not perfect, I think that approach strikes the right balance
----------------------------------------------------------------
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] zhangmeng916 commented on pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on pull request #1033:
URL: https://github.com/apache/helix/pull/1033#issuecomment-638539425
This PR is ready to merge. Approved by @pkuwm
Final commit message:
Remove stack trace when customized state root does not exist and throws no node exception.
----------------------------------------------------------------
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] zhangmeng916 commented on a change in pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r434910383
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -591,8 +591,11 @@ public void addCurrentStateChangeListener(org.apache.helix.CurrentStateChangeLis
@Override
public void addCustomizedStateRootChangeListener(CustomizedStateRootChangeListener listener,
String instanceName) throws Exception {
- addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
- ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ PropertyKey propertyKey = _dataAccessor.keyBuilder().customizedStatesRoot(instanceName);
+ if (_zkclient.exists(propertyKey.getPath())) {
Review comment:
I updated this rb with a solution to avoid stack trace in our customers who have not updated their helix-lib version. As discussed before, this is the simplest while robust way to solve the issue without performance penalty. For further improvement on customized state listener, we track in this issue: #1046 and tackle it later.
----------------------------------------------------------------
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] zhangmeng916 commented on a change in pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r435455066
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -639,14 +639,20 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
}
}
} catch (ZkNoNodeException | HelixMetaDataAccessException e) {
- logger.warn(
- "fail to subscribe child/data change. path: " + path + ", listener: " + _listener, e);
+ // TODO: avoid calling getChildren for path that does not exist
+ if (_changeType == CUSTOMIZED_STATE_ROOT) {
+ logger.warn("Failed to subscribe child/data change. path: {}, listener: {}", path,
+ _listener);
+ } else {
+ logger.warn("Failed to subscribe child/data change. path: {}, listener: {}", path,
+ _listener, e);
Review comment:
@narendly tried to remove duplicates. However, this will make the log and stacktrace to be in two entries. Won't look good.
----------------------------------------------------------------
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 #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r436040023
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -639,14 +639,18 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
}
}
} catch (ZkNoNodeException | HelixMetaDataAccessException e) {
- logger.warn(
- "fail to subscribe child/data change. path: " + path + ", listener: " + _listener, e);
+ logger.warn("Failed to subscribe child/data change. path: {}, listener: {}", path,
+ _listener);
+ // TODO: avoid calling getChildren for path that does not exist
+ if (_changeType != CUSTOMIZED_STATE_ROOT) {
+ logger.warn("", e);
+ }
Review comment:
```
if (_changeType != CUSTOMIZED_STATE_ROOT) {
LOG.warn("Failed to subscribe .... Instance doesn't support Customized State!, path, _listerner);
} else {
LOG.warn("Failed to.... ");
}
```
How about this? this way you don't print all the exceptions stack trace, and it doesn't look like it's duplicate code, and the message is clear.
----------------------------------------------------------------
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 pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
narendly commented on pull request #1033:
URL: https://github.com/apache/helix/pull/1033#issuecomment-635553574
Here's the copy of an offline discussion
> Jiajun Wang:man-woman-girl-girl: 12:27
> Meng will confirm. But I think that scenario was discussed and address in the original design.
> 12:28
> I will suggest keeping the root path listener (default feature for the new version controller) but the children's paths listeners will depend on the configuration.
>
>
>
>
>
> 12:29
> that is the minimum code change and logic is the simpliest.
> 12:30
> Basically, we only need to avoid the default subscription to children nodes in the root path subscribe process (edited)
>
> Hunter Lee 12:30
> So are we comfortable with creating CallbackHandlers for nonexistent root paths?
>
> Jiajun Wang 12:30
> Then all the rest parts have been handled by the current code.
> New
> 12:32
> @hulee That's the only downside. But my argument is that:
> It is a default feature for the new controller.
> It won't cause watcher leakage. It is always associated with a live callback handler, although never triggered,
> The new participant will always have this folder. That's the agreement.
> 12:33
> The alternative design will create many additional codes for the "perfect" backward compatibility.
> 12:33
> Which is not necessary IMO
>
> Hunter Lee 12:35
> Ok. Although not perfect, I think that approach strikes the right balance
----------------------------------------------------------------
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] dasahcc commented on a change in pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r432734931
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -591,8 +591,11 @@ public void addCurrentStateChangeListener(org.apache.helix.CurrentStateChangeLis
@Override
public void addCustomizedStateRootChangeListener(CustomizedStateRootChangeListener listener,
String instanceName) throws Exception {
- addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
- ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ PropertyKey propertyKey = _dataAccessor.keyBuilder().customizedStatesRoot(instanceName);
+ if (_zkclient.exists(propertyKey.getPath())) {
Review comment:
Shall we have some logs to indicate it is not able to subscribe? Maybe warning or something?
----------------------------------------------------------------
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] zhangmeng916 commented on pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on pull request #1033:
URL: https://github.com/apache/helix/pull/1033#issuecomment-637108731
I updated this rb with a solution to avoid stack trace in our customers who have not updated their helix-lib version. As discussed before, this is the simplest while robust way to solve the issue without performance penalty. For further improvement on customized state listener, we track in this issue: https://github.com/apache/helix/issues/1046 and tackle it later.
----------------------------------------------------------------
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] dasahcc commented on a change in pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r432734931
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -591,8 +591,11 @@ public void addCurrentStateChangeListener(org.apache.helix.CurrentStateChangeLis
@Override
public void addCustomizedStateRootChangeListener(CustomizedStateRootChangeListener listener,
String instanceName) throws Exception {
- addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
- ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ PropertyKey propertyKey = _dataAccessor.keyBuilder().customizedStatesRoot(instanceName);
+ if (_zkclient.exists(propertyKey.getPath())) {
Review comment:
Shall we have some logs to indicate it is not able to subscribe? Maybe warning or something? Because after adding this check, it will be silently ignore the subscription.
----------------------------------------------------------------
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] pkuwm commented on a change in pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r433529334
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1258,12 +1258,12 @@ protected void checkLiveInstancesObservation(List<LiveInstance> liveInstances,
if (lastInstances == null || !lastInstances.containsKey(instance)) {
try {
manager.addCustomizedStateRootChangeListener(this, instance);
- logger.info(manager.getInstanceName() + " added customized root change listener for"
- + " " + instance
- + ", listener: " + this);
+ logger.info(manager.getInstanceName() + " added root path listener for customized "
Review comment:
I understand you may want to keep logging as original. But since you are changing the logging code here, I suggest changing it to use parameterized logging: `logger.info("Hello {}", world);`. This parameterized logging has benefits of reduce string concatenation overhead when logging level info is not enabled, and easier to read in the code. Same as the other places in this PR.
Nit, I realized this for loop needs unindent.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -639,8 +638,13 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
}
}
} catch (ZkNoNodeException | HelixMetaDataAccessException e) {
- logger.warn(
- "fail to subscribe child/data change. path: " + path + ", listener: " + _listener, e);
+ if (_changeType == CUSTOMIZED_STATE_ROOT) {
+ logger.warn(
+ "fail to subscribe child/data change. path: " + path + ", listener: " + _listener);
Review comment:
`fail to` -> `Failed to` ? Same for the others.
----------------------------------------------------------------
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 merged pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
narendly merged pull request #1033:
URL: https://github.com/apache/helix/pull/1033
----------------------------------------------------------------
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] pkuwm commented on a change in pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r431535615
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -591,8 +591,11 @@ public void addCurrentStateChangeListener(org.apache.helix.CurrentStateChangeLis
@Override
public void addCustomizedStateRootChangeListener(CustomizedStateRootChangeListener listener,
String instanceName) throws Exception {
- addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
- ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ PropertyKey propertyKey = _dataAccessor.keyBuilder().customizedStatesRoot(instanceName);
+ if (_zkclient.exists(propertyKey.getPath())) {
Review comment:
It is just a warn log. I think it is OK not to print the stack trace. And if the stack trace is necessary, we could keep the stack trace for the other paths, but for this customized state path, just log the message.
----------------------------------------------------------------
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] zhangmeng916 commented on a change in pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r431502620
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -591,8 +591,11 @@ public void addCurrentStateChangeListener(org.apache.helix.CurrentStateChangeLis
@Override
public void addCustomizedStateRootChangeListener(CustomizedStateRootChangeListener listener,
String instanceName) throws Exception {
- addListener(listener, new Builder(_clusterName).customizedStatesRoot(instanceName),
- ChangeType.CUSTOMIZED_STATE_ROOT, new EventType[]{EventType.NodeChildrenChanged});
+ PropertyKey propertyKey = _dataAccessor.keyBuilder().customizedStatesRoot(instanceName);
+ if (_zkclient.exists(propertyKey.getPath())) {
Review comment:
If we don't check path, the only way I can think of is to check participant side version. Doesn't sounds a clean way 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] narendly commented on a change in pull request #1033: Add path existing check for customized state
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1033:
URL: https://github.com/apache/helix/pull/1033#discussion_r431483789
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -1256,14 +1256,18 @@ protected void checkLiveInstancesObservation(List<LiveInstance> liveInstances,
for (String instance : curInstances.keySet()) {
if (lastInstances == null || !lastInstances.containsKey(instance)) {
- try {
- manager.addCustomizedStateRootChangeListener(this, instance);
- logger.info(manager.getInstanceName() + " added customized root change listener for"
- + " " + instance
- + ", listener: " + this);
- } catch (Exception e) {
- logger.error("Fail to add customized root change listener for instance: " + instance,
- e);
+ PropertyKey propertyKey =
+ keyBuilder.customizedStatesRoot(instance);
+ if (propertyKey.getPath() != null) {
Review comment:
You're creating a PropertyKey object with the given instance name and checking whether it's null? How would it be null if you've just created the path? In other words, when would this condition ever evaluate to false?
Did you mean to check whether the path exists in ZooKeeper? If so, shouldn't you do an exist() check with a ZkClient-based API?
Unless I'm mistaken and we actually somehow pre-populate the KeyBuilder with a whitelist of instances for the Customized State feature and returns null if the instance doesn't show up in the list?
----------------------------------------------------------------
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