[
https://issues.apache.org/jira/browse/ZOOKEEPER-442?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13867138#comment-13867138
]
Raul Gutierrez Segales commented on ZOOKEEPER-442:
--------------------------------------------------
Sorry for the late response, [~rakeshr]. Thanks for the update. Here are a few
more comments:
Perf nit in src/java/main/org/apache/zookeeper/ClientCnxn.java:
{noformat}
+ Set<Watcher> watchers = new HashSet<Watcher>();
+ if (materializedWatchers == null) {
+ // materialize the watchers based on the event
+ watchers = watcher.materialize(event.getState(),
+ event.getType(), event.getPath());
+ } else {
+ watchers.addAll(materializedWatchers);
+ }
+ WatcherSetEventPair pair = new WatcherSetEventPair(watchers,
event);
{noformat}
If materializedWatchers == null then calling watcher.materialize() will give
you a new Set of watchers (overriding the previously instantiated HashSet). So
you should only instantiate new HashSet<Watcher>() inside the else clause.
In src/java/main/org/apache/zookeeper/ZooKeeper.java:
{noformat}
+ public Map<EventType, Set<Watcher>> removeWatcher(String clientPath,
+ Watcher watcher, WatcherType watcherType, boolean local, int
rc)
+ throws KeeperException {
....
+ removedWatchers
+ .put(EventType.ChildWatchRemoved, childWatchersToRem);
....
+ case Any: {
+ removedWatchers.put(EventType.ChildWatchRemoved,
+ childWatchersToRem);
{noformat}
The call to removedWatchers.put(EventType.ChildWatchRemoved,
childWatchersToRem) inside the Any switch clause doesn't seem to be needed, no?
Nit in:
{noformat}
+ public void removeWatches(String path, Watcher watcher,
+ WatcherType watcherType, boolean local)
+ throws InterruptedException, KeeperException {
+ final String clientPath = path;
{noformat}
why is assigning to clientPath needed? In the previous comments the path param
is referred as clientPath anyway, maybe renamed it?
In:
{noformat}
+ // Validating the existence of wacher, because there could be a case
+ // where the watcher might be triggered just before receiving the
remove
+ // watch response.
+ try {
+ watchManager.containsWatcher(clientPath, watcher, watcherType);
+ } catch (NoWatcherException nwe) {
+ LOG.error("Failed to find watcher!", nwe);
+ cb.processResult(nwe.code().intValue(), clientPath, ctx);
+ return;
+ }
{noformat}
I think we should just throw NoWatcherException otherwise how can the caller of
this (async) method know if it failed to be scheduled? I think this is how
other async methods behave when the remote call fails to be issued. They either
raise an exception or return an error code instead of having their callback
being called. The callback should only be called if the remote call was
performed (so then it only gets server generated errors, if any).
Besides these points, I am happy to give it a +1. Hope to see this merged soon!
Awesome work.
> need a way to remove watches that are no longer of interest
> -----------------------------------------------------------
>
> Key: ZOOKEEPER-442
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-442
> Project: ZooKeeper
> Issue Type: Sub-task
> Components: java client, server
> Reporter: Benjamin Reed
> Assignee: Rakesh R
> Priority: Critical
> Fix For: 3.5.0
>
> Attachments: Remove Watch API.pdf, ZOOKEEPER-442.patch,
> ZOOKEEPER-442.patch, ZOOKEEPER-442.patch, ZOOKEEPER-442.patch,
> ZOOKEEPER-442.patch, ZOOKEEPER-442.patch, ZOOKEEPER-442.patch,
> ZOOKEEPER-442.patch, ZOOKEEPER-442.patch, ZOOKEEPER-442.patch,
> ZOOKEEPER-442.patch
>
>
> currently the only way a watch cleared is to trigger it. we need a way to
> enumerate the outstanding watch objects, find watch events the objects are
> watching for, and remove interests in an event.
--
This message was sent by Atlassian JIRA
(v6.1.5#6160)
