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