i3wangyi commented on a change in pull request #688: Fix the watcher leakage
issue
URL: https://github.com/apache/helix/pull/688#discussion_r376117446
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/zookeeper/ZkClient.java
##########
@@ -960,10 +956,10 @@ public void process(WatchedEvent event) {
private void fireAllEvents() {
for (Entry<String, Set<IZkChildListener>> entry :
_childListener.entrySet()) {
- fireChildChangedEvents(entry.getKey(), entry.getValue());
+ fireChildChangedEvents(entry.getKey(), entry.getValue(), true);
Review comment:
1. I don't think to bump up the ZK version and use the new API to delete ZK
watcher explicitly would resolve the problem.
Let me explain why: if the node is re-created after deleting the watcher
request is submitted, without the parent path's child listener, there's no way
the ZkClient would be able to receive new events from the path. It's the same
dilemma this fix is currently facing.
2. A background GC thread is an over-kill solution. First It's difficult to
determine the right TTL; Second, it comes with the additional thread cost.
My argument point is that it's not always valid to say we need to be 100%
stick to the old behavior, especially when the old behavior is incorrect and
leading to the excessive amount of leakage.
The easiest way is to release a newer Helix version and educate the client
with the caveat **"Once a node is deleted, the listener on the path will be
inactive even the node is re-created; The workaround is to have a child
listener on the parent path or re-subscribe the path"**. The statement IMO
makes sense to most clients.
As to how many clients will be suffering from the behavior change, I don't
have a good answer but the experience told me the number is minor if the client
is using Helix as a cluster management tool instead of a generic ZKClient
proxy.
Do these all make sense? @kaisun2000 @dasahcc please feel free to leave any
thoughts about this, cause it will change the design fundamentally and won't be
able to help Venice reduce the watcher pains in any near future.
----------------------------------------------------------------
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]
