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/09/03 05:25:29 UTC
[GitHub] [helix] pkuwm opened a new pull request #1344: Reduce unnecessary getChildren call in subscribeForChanges
pkuwm opened a new pull request #1344:
URL: https://github.com/apache/helix/pull/1344
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1343
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
In CallbackHanlder's subscribeForChanges(), children names need to be fetched. subscribeChildChange(path, callbackType) does actually have the children list, but it doesn't return, and later getChildren() is called again. It wastes one zk call: not only wasting time in handling a callback, but also adds more read pressure to zk server.
We could use the return result from subscribeChildChange() and save one getChildren() call.
This PR also adds event types logging and removes two logs. When debugging, we can actually use the event types to determine the code logic. No need to print those two logs, which saves logging overhead when there are intensive callbacks.
### Tests
- [ ] The following tests are written for this issue:
(List the names of added unit/integration tests)
- [ ] The following is the result of the "mvn test" command on the appropriate module:
(Before CI test pass, please copy & paste the result of "mvn test")
### 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] pkuwm commented on a change in pull request #1344: Reduce unnecessary getChildren call in subscribeForChanges
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1344:
URL: https://github.com/apache/helix/pull/1344#discussion_r483282771
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -533,7 +533,7 @@ public void invoke(NotificationContext changeContext) throws Exception {
}
}
- private void subscribeChildChange(String path, NotificationContext.Type callbackType) {
+ private List<String> subscribeChildChange(String path, NotificationContext.Type callbackType) {
if (callbackType == NotificationContext.Type.INIT
Review comment:
I also thought about it. But from the code, if `_isInstalled == true`, it is still possible that the children list is null:
```
try {
return getChildren(path, true);
} catch (ZkNoNodeException e) {
// ignore, the "exists" watch will listen for the parent node to appear
LOG.info("watchForChilds path not existing:{} skipWatchingNodeNoteExist: {}",
path, skipWatchingNonExistNode);
}
return null;
```
So we could not determine if children list is null or not based on `_isInstalled`.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -592,8 +595,8 @@ private void subscribeForChangesAsyn(NotificationContext.Type callbackType, Stri
private void subscribeForChanges(NotificationContext.Type callbackType, String path,
boolean watchChild) {
- logger.info("Subscribing changes listener to path: {}, type: {}, listener: {}", path,
- callbackType, _listener);
+ logger.info("Subscribing changes listener to path: {}, callback type: {}, event types: {}, "
Review comment:
Actually when grepping, `Subscribing changes listener to path: ` would be good enough. Those variables could change for different callback and paths. we could grep for a specific string. Anyway, I'll address this to append the new fields to the end.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -604,11 +607,8 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
}
if (_eventTypes.contains(EventType.NodeChildrenChanged)) {
- logger.info("Subscribing child change listener to path: {}", path);
Review comment:
From my debug experience, these 2 logs are distracting and not helping much. The import log we want is event type. We can use event type to determine the code logic here. And these 2 add too many lines of logs to the log file. So I prefer to remove them and instead log the event types.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -646,10 +645,14 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
break;
}
default: {
- List<String> childNames = _zkClient.getChildren(path);
- if (childNames != null) {
- for (String childName : childNames) {
- String childPath = path + "/" + childName;
+ // When callback type is FINALIZE, subscribeChildChange doesn't
+ // get children list. We need to read children to unsubscribe data change listeners.
+ if (callbackType == Type.FINALIZE) {
+ children = _zkClient.getChildren(path);
+ }
+ if (children != null) {
+ for (String child : children) {
Review comment:
async processing this will have more changes. I'll leave that later in another PR.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -554,12 +554,15 @@ private void subscribeChildChange(String path, NotificationContext.Type callback
if (!childrenSubscribeResult.isInstalled()) {
logger.info("CallbackHandler {} subscribe data path {} failed!", this, path);
}
+ return childrenSubscribeResult.getChildren();
Review comment:
`subscribeChildChanges` returns null when the parent path doesn't exist.
About the potential race condition, we can't rely on later `getChildren` to fetch children node if subscribeChildChanges returns null. Between these two calls, the time window is very tiny. And I believe the system could handle the change. If first is null (parent node does not exist), later it exists, we should be able to have another callback to handle.
----------------------------------------------------------------
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 #1344: Reduce unnecessary getChildren call in subscribeForChanges
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1344:
URL: https://github.com/apache/helix/pull/1344#discussion_r483185386
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -646,10 +645,9 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
break;
}
default: {
- List<String> childNames = _zkClient.getChildren(path);
- if (childNames != null) {
- for (String childName : childNames) {
- String childPath = path + "/" + childName;
+ if (children != null) {
Review comment:
Yeah, I was aware of the failed test and the code in `subscribeChildChange()` for `NotificationContext.Type.FINALIZE`.
----------------------------------------------------------------
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] xyuanlu commented on a change in pull request #1344: Reduce unnecessary getChildren call in subscribeForChanges
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #1344:
URL: https://github.com/apache/helix/pull/1344#discussion_r482715694
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -533,7 +533,7 @@ public void invoke(NotificationContext changeContext) throws Exception {
}
}
- private void subscribeChildChange(String path, NotificationContext.Type callbackType) {
+ private List<String> subscribeChildChange(String path, NotificationContext.Type callbackType) {
if (callbackType == NotificationContext.Type.INIT
Review comment:
I feel like we could create an empty list here. So we won’t return a null and no need to do the nullptr check 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] xyuanlu commented on a change in pull request #1344: Reduce unnecessary getChildren call in subscribeForChanges
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #1344:
URL: https://github.com/apache/helix/pull/1344#discussion_r482792770
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -646,10 +645,9 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
break;
}
default: {
- List<String> childNames = _zkClient.getChildren(path);
- if (childNames != null) {
- for (String childName : childNames) {
- String childPath = path + "/" + childName;
+ if (children != null) {
Review comment:
There is a slightly behavior difference here. Originally before this change, if type is Finalize, we still iterate trough all node and do `subscribeDataChange `.
Now we do not return a children list when finalize (line 565) so we do not do `subscribeDataChange` for those nodes.
I think we need to keep the getChildren when finalize. Also, This behavior difference cause unit test `TestListenerCallbackBatchMode` fail on my side. May I ask did you run in to this problem?
Thx.
----------------------------------------------------------------
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 #1344: Reduce unnecessary getChildren call in subscribeForChanges
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1344:
URL: https://github.com/apache/helix/pull/1344#discussion_r494690185
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -538,7 +538,12 @@ public void invoke(NotificationContext changeContext) throws Exception {
}
}
- private void subscribeChildChange(String path, NotificationContext.Type callbackType) {
+ /*
+ * If callback type is INIT or CALLBACK, subscribes child change listener to the path
+ * and returns the path's children names. The children list might be null when the path
+ * doesn't exist or callback type is other than INIT/CALLBACK/FINALIZE.
Review comment:
The current logic will return a list even the type is not INIT/CALLBACK/FINALIZE, right? Although I guess it is expected.
1. Please fix the comment.
2. The method name becomes misleading somehow with this change. I guess it should be called getChildrenAndSubscribeChildChange. Obviously, not a good name. But feel free to change if you have a better idea. This is just a nice to have thing.
----------------------------------------------------------------
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 #1344: Reduce unnecessary getChildren call in subscribeForChanges
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1344:
URL: https://github.com/apache/helix/pull/1344#discussion_r494690185
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -538,7 +538,12 @@ public void invoke(NotificationContext changeContext) throws Exception {
}
}
- private void subscribeChildChange(String path, NotificationContext.Type callbackType) {
+ /*
+ * If callback type is INIT or CALLBACK, subscribes child change listener to the path
+ * and returns the path's children names. The children list might be null when the path
+ * doesn't exist or callback type is other than INIT/CALLBACK/FINALIZE.
Review comment:
The current logic will return a list even the type is not INIT/CALLBACK/FINALIZE, right? Although I guess it is expected.
1. Please fix the comment.
2. The method name becomes misleading somehow with this change. I guess it should be called getChildrenAndSubscribeChildChange. Obviously, not a good name. But feel free to change if you have a better idea. This is just a nice to have thing.
----------------------------------------------------------------
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 merged pull request #1344: Reduce unnecessary getChildren call in subscribeForChanges
Posted by GitBox <gi...@apache.org>.
pkuwm merged pull request #1344:
URL: https://github.com/apache/helix/pull/1344
----------------------------------------------------------------
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 merged pull request #1344: Reduce unnecessary getChildren call in subscribeForChanges
Posted by GitBox <gi...@apache.org>.
pkuwm merged pull request #1344:
URL: https://github.com/apache/helix/pull/1344
----------------------------------------------------------------
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 #1344: Reduce unnecessary getChildren call in subscribeForChanges
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1344:
URL: https://github.com/apache/helix/pull/1344#discussion_r484530705
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -646,10 +645,14 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
break;
}
default: {
- List<String> childNames = _zkClient.getChildren(path);
- if (childNames != null) {
- for (String childName : childNames) {
- String childPath = path + "/" + childName;
+ // When callback type is FINALIZE or PERIODIC_REFRESH, subscribeChildChange doesn't
+ // get children list. We need to read children to unsubscribe data change listeners.
+ if (callbackType != Type.INIT && callbackType != Type.CALLBACK) {
Review comment:
Replied above.
----------------------------------------------------------------
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 #1344: Reduce unnecessary getChildren call in subscribeForChanges
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1344:
URL: https://github.com/apache/helix/pull/1344#discussion_r483226102
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -554,12 +554,15 @@ private void subscribeChildChange(String path, NotificationContext.Type callback
if (!childrenSubscribeResult.isInstalled()) {
logger.info("CallbackHandler {} subscribe data path {} failed!", this, path);
}
+ return childrenSubscribeResult.getChildren();
Review comment:
I think the main issue here is that line 551
`_zkClient.subscribeChildChanges(path, this, callbackType != Type.INIT);` we have two code path.
if callbackType is init, the parent path of callback handler may not be created. Thus, the return would be null, or empty? Please verify.
The potentially issue is that note later at `default`, we re-use the return children list from here.
Note, original code retrieve children one more time and it may retrieve node by then. Here, it may not.
This is different from previous logic. Not sure if this would bring a bug or 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] pkuwm commented on pull request #1344: Reduce unnecessary getChildren call in subscribeForChanges
Posted by GitBox <gi...@apache.org>.
pkuwm commented on pull request #1344:
URL: https://github.com/apache/helix/pull/1344#issuecomment-698712073
This PR is ready to be merged, approved by @jiajunwang
In CallbackHanlder's subscribeForChanges(), children names need to be fetched. subscribeChildChange(path, callbackType) does actually have the children list, but it doesn't return, and later getChildren() is called again. It wastes one zk call: not only wasting time in handling a callback, but also adds more read pressure to zk server.
This commit uses the return result from subscribeChildChange() and saves getChildren() call.
----------------------------------------------------------------
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 #1344: Reduce unnecessary getChildren call in subscribeForChanges
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1344:
URL: https://github.com/apache/helix/pull/1344#discussion_r494694946
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -538,7 +538,12 @@ public void invoke(NotificationContext changeContext) throws Exception {
}
}
- private void subscribeChildChange(String path, NotificationContext.Type callbackType) {
+ /*
+ * If callback type is INIT or CALLBACK, subscribes child change listener to the path
+ * and returns the path's children names. The children list might be null when the path
+ * doesn't exist or callback type is other than INIT/CALLBACK/FINALIZE.
Review comment:
Resolved comment.
Since we also have `subscribeChildChanges()` in ZkClient that returns children, so I'd like to keep it as it is to be consistent. Besides, I don't have a better name for this and I think it is OK to keep 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] xyuanlu commented on a change in pull request #1344: Reduce unnecessary getChildren call in subscribeForChanges
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #1344:
URL: https://github.com/apache/helix/pull/1344#discussion_r482792770
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -646,10 +645,9 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
break;
}
default: {
- List<String> childNames = _zkClient.getChildren(path);
- if (childNames != null) {
- for (String childName : childNames) {
- String childPath = path + "/" + childName;
+ if (children != null) {
Review comment:
There is a slightly behavior difference here. Originally before this change, if type is Finalize, we still iterate trough all node and do `subscribeDataChange `.
Now we do not return a children list when finalize (line 565) so we do not do `subscribeDataChange` for those nodes.
This behavior difference cause unit test `TestListenerCallbackBatchMode` fail on my side. Did you run in to this problem?
Thx.
----------------------------------------------------------------
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 pull request #1344: Reduce unnecessary getChildren call in subscribeForChanges
Posted by GitBox <gi...@apache.org>.
pkuwm commented on pull request #1344:
URL: https://github.com/apache/helix/pull/1344#issuecomment-698712073
This PR is ready to be merged, approved by @jiajunwang
In CallbackHanlder's subscribeForChanges(), children names need to be fetched. subscribeChildChange(path, callbackType) does actually have the children list, but it doesn't return, and later getChildren() is called again. It wastes one zk call: not only wasting time in handling a callback, but also adds more read pressure to zk server.
This commit uses the return result from subscribeChildChange() and saves getChildren() call.
----------------------------------------------------------------
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 #1344: Reduce unnecessary getChildren call in subscribeForChanges
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1344:
URL: https://github.com/apache/helix/pull/1344#discussion_r489690588
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -646,10 +652,14 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
break;
}
default: {
- List<String> childNames = _zkClient.getChildren(path);
- if (childNames != null) {
- for (String childName : childNames) {
- String childPath = path + "/" + childName;
+ // When callback type is FINALIZE, subscribeChildChange doesn't
+ // get children list. We need to read children to unsubscribe data change listeners.
+ if (callbackType == Type.FINALIZE) {
+ children = _zkClient.getChildren(path);
+ }
Review comment:
As we discussed, I think this check is not necessary. The later check for children != null is good enough.
If the node was not created and then create during this callback processing, then we will have another callback to process the event since the watcher has been added.
----------------------------------------------------------------
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 #1344: Reduce unnecessary getChildren call in subscribeForChanges
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1344:
URL: https://github.com/apache/helix/pull/1344#discussion_r483169343
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -533,7 +533,7 @@ public void invoke(NotificationContext changeContext) throws Exception {
}
}
- private void subscribeChildChange(String path, NotificationContext.Type callbackType) {
+ private List<String> subscribeChildChange(String path, NotificationContext.Type callbackType) {
Review comment:
Method comment, please.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -592,8 +595,8 @@ private void subscribeForChangesAsyn(NotificationContext.Type callbackType, Stri
private void subscribeForChanges(NotificationContext.Type callbackType, String path,
boolean watchChild) {
- logger.info("Subscribing changes listener to path: {}, type: {}, listener: {}", path,
- callbackType, _listener);
+ logger.info("Subscribing changes listener to path: {}, callback type: {}, event types: {}, "
Review comment:
For debug, please add new fields to the end of the string. And don't change the existing content if they are not hurt too much. Otherwise, the dev needs to switch the code version back and forth to double-check before debugging the older version deployment.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -604,11 +607,8 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
}
if (_eventTypes.contains(EventType.NodeChildrenChanged)) {
- logger.info("Subscribing child change listener to path: {}", path);
Review comment:
I personally think these 2 info logs are still desired. Especially we are changing the subscription mechanism.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -533,7 +533,7 @@ public void invoke(NotificationContext changeContext) throws Exception {
}
}
- private void subscribeChildChange(String path, NotificationContext.Type callbackType) {
+ private List<String> subscribeChildChange(String path, NotificationContext.Type callbackType) {
if (callbackType == NotificationContext.Type.INIT
Review comment:
Is it possible that you just return ChildrenSubscribeResult?
Then if _isInstalled is false, no one will read the children. Otherwise, if _isInstalled, then the list should never be null.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -646,10 +645,14 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
break;
}
default: {
- List<String> childNames = _zkClient.getChildren(path);
- if (childNames != null) {
- for (String childName : childNames) {
- String childPath = path + "/" + childName;
+ // When callback type is FINALIZE or PERIODIC_REFRESH, subscribeChildChange doesn't
+ // get children list. We need to read children to unsubscribe data change listeners.
+ if (callbackType != Type.INIT && callbackType != Type.CALLBACK) {
Review comment:
As I mentioned above, if the return value is ChildrenSubscribeResult, then you just need to check "_isInstalled" for the following logic.
----------------------------------------------------------------
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 #1344: Reduce unnecessary getChildren call in subscribeForChanges
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1344:
URL: https://github.com/apache/helix/pull/1344#discussion_r494636237
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -646,10 +652,14 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
break;
}
default: {
- List<String> childNames = _zkClient.getChildren(path);
- if (childNames != null) {
- for (String childName : childNames) {
- String childPath = path + "/" + childName;
+ // When callback type is FINALIZE, subscribeChildChange doesn't
+ // get children list. We need to read children to unsubscribe data change listeners.
+ if (callbackType == Type.FINALIZE) {
+ children = _zkClient.getChildren(path);
+ }
Review comment:
Moved the logic to subscribeChildChange: for FINALIZE, it also returns the children.
----------------------------------------------------------------
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 #1344: Reduce unnecessary getChildren call in subscribeForChanges
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1344:
URL: https://github.com/apache/helix/pull/1344#discussion_r482739928
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -533,7 +533,7 @@ public void invoke(NotificationContext changeContext) throws Exception {
}
}
- private void subscribeChildChange(String path, NotificationContext.Type callbackType) {
+ private List<String> subscribeChildChange(String path, NotificationContext.Type callbackType) {
if (callbackType == NotificationContext.Type.INIT
Review comment:
I thought about it. There is still a possibility that `childrenSubscribeResult.getChildren()` returns `null`. The caller still needs to do the `null` check. So I just make it consistent with `zkClient.subscribeChildChanges` and let the caller determine the behavior.
I also thought about using `Optional`, but it doesn't seem that would add too much value. And `Optional` will add more overhead than `null`. The original code also does null check, so for now I'd keep the change minimum. Later we will still have a better design and hopefully will make the code cleaner.
----------------------------------------------------------------
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 #1344: Reduce unnecessary getChildren call in subscribeForChanges
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1344:
URL: https://github.com/apache/helix/pull/1344#discussion_r494636237
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -646,10 +652,14 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
break;
}
default: {
- List<String> childNames = _zkClient.getChildren(path);
- if (childNames != null) {
- for (String childName : childNames) {
- String childPath = path + "/" + childName;
+ // When callback type is FINALIZE, subscribeChildChange doesn't
+ // get children list. We need to read children to unsubscribe data change listeners.
+ if (callbackType == Type.FINALIZE) {
+ children = _zkClient.getChildren(path);
+ }
Review comment:
Moved the logic to subscribeChildChange: for FINALIZE, it also returns the children.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -538,7 +538,12 @@ public void invoke(NotificationContext changeContext) throws Exception {
}
}
- private void subscribeChildChange(String path, NotificationContext.Type callbackType) {
+ /*
+ * If callback type is INIT or CALLBACK, subscribes child change listener to the path
+ * and returns the path's children names. The children list might be null when the path
+ * doesn't exist or callback type is other than INIT/CALLBACK/FINALIZE.
Review comment:
Resolved comment.
Since we also have `subscribeChildChanges()` in ZkClient that returns children, so I'd like to keep it as it is to be consistent. Besides, I don't have a better name for this and I think it is OK to keep 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] kaisun2000 commented on a change in pull request #1344: Reduce unnecessary getChildren call in subscribeForChanges
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1344:
URL: https://github.com/apache/helix/pull/1344#discussion_r483224509
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -646,10 +645,14 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
break;
}
default: {
- List<String> childNames = _zkClient.getChildren(path);
- if (childNames != null) {
- for (String childName : childNames) {
- String childPath = path + "/" + childName;
+ // When callback type is FINALIZE, subscribeChildChange doesn't
+ // get children list. We need to read children to unsubscribe data change listeners.
+ if (callbackType == Type.FINALIZE) {
+ children = _zkClient.getChildren(path);
+ }
+ if (children != null) {
+ for (String child : children) {
Review comment:
Parallel this using Asyc should bring more performance gain, right? why don't we do it 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