i3wangyi commented on a change in pull request #688: Fix the watcher leakage
issue
URL: https://github.com/apache/helix/pull/688#discussion_r368196993
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/zookeeper/ZkClient.java
##########
@@ -1261,31 +1258,62 @@ private void processDataOrChildChange(WatchedEvent
event, long notificationTime)
if (event.getType() == EventType.NodeDataChanged || event.getType() ==
EventType.NodeDeleted
|| event.getType() == EventType.NodeCreated) {
+ boolean isPathExist = event.getType() != EventType.NodeDeleted;
Set<IZkDataListenerEntry> listeners = _dataListener.get(path);
if (listeners != null && !listeners.isEmpty()) {
- fireDataChangedEvents(event.getPath(), listeners,
OptionalLong.of(notificationTime));
+ fireDataChangedEvents(event.getPath(), listeners, notificationTime,
isPathExist);
}
}
}
+ private void fireDataChangedEvents(final String path,
Set<IZkDataListenerEntry> listeners) {
+ for (final IZkDataListenerEntry listener : listeners) {
+ ZkEvent zkEvent = new ZkEvent(
+ "Data of " + path + " changed sent to " + listener.getDataListener()
+ " prefetch data: "
+ + listener.isPrefetchData()) {
+ @Override
+ public void run()
+ throws Exception {
+ Object data;
+ // TODO: are we fetching the data multiple times?
+ if (listener.isPrefetchData()) {
+ LOG.debug("Prefetch data for path: {}", path);
+ try {
+ data = readData(path, null, true);
+ } catch (ZkNoNodeException e) {
+ LOG.warn("Prefetch data for path: {} failed.", path, e);
+ listener.getDataListener().handleDataDeleted(path);
+ return;
+ }
+ listener.getDataListener().handleDataChange(path, data);
+ }
+ }
+ };
+
+ _eventThread.send(zkEvent);
+ }
+ }
+
private void fireDataChangedEvents(final String path,
Set<IZkDataListenerEntry> listeners,
- final OptionalLong notificationTime) {
+ final long notificationTime, final boolean isPathExist) {
+ final ZkPathStatRecord pathStatRecord = new ZkPathStatRecord(path);
Review comment:
The truth is: currently both *exists* listener and *child* listener are
listening on the path (e.g /IDEALSTATES/db32). When the db32 path is removed,
both types of listeners will get triggered and they should be (cannot avoid it)
unless doing additional checks on the path (check if it's Helix main path or
not).
The child change handler needs to handle the child node change. Then a
re-installation of a child handler needs to happen.
The data change handler also gets triggered to handle the data change. In
the deletion case, because of the existing code, we'll add the unwanted watcher
again onto the deleted path. With the fix, it will avoid adding the unwanted
watcher and the logic applies to every path in Znode
----------------------------------------------------------------
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]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
