----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3364/#review4182 -----------------------------------------------------------
This is only a first pass. Looks ok but I would like more attention to anything you're not really sure about. src/java/main/org/apache/zookeeper/ZooKeeper.java <https://reviews.apache.org/r/3364/#comment9414> Maybe collapse this to if(watchers != null && watchers.contains(watcher)) ? src/java/main/org/apache/zookeeper/ZooKeeper.java <https://reviews.apache.org/r/3364/#comment9415> I don't think that you should override this method to do two things based on the variables passed in, it's confusing and unnecessary. Can we have instead two methods, one for removing all watchers and one for just removing a watch if it exists? src/java/main/org/apache/zookeeper/ZooKeeper.java <https://reviews.apache.org/r/3364/#comment9416> ?? src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java <https://reviews.apache.org/r/3364/#comment9417> Let's not check in files that are just reordering of imports... - Camille On 2012-01-04 09:41:43, Daniel Gómez Ferro wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3364/ > ----------------------------------------------------------- > > (Updated 2012-01-04 09:41:43) > > > Review request for zookeeper and Benjamin Reed. > > > Summary > ------- > > Added APIs to remove watches that are not needed anymore. If the removal > completes successfully it is guaranteed that the watcher won't be notified. > > With the current semantics if two clients remove watches on a znode at the > same time the watch is triggered, one could remove it successfully while the > other could receive the notification first. > > > This addresses bug ZOOKEEPER-442. > https://issues.apache.org/jira/browse/ZOOKEEPER-442 > > > Diffs > ----- > > src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40 > src/java/main/org/apache/zookeeper/KeeperException.java 7c10d2c > src/java/main/org/apache/zookeeper/Watcher.java 36c7b5b > src/java/main/org/apache/zookeeper/ZooDefs.java c7f1b20 > src/java/main/org/apache/zookeeper/ZooKeeper.java e82eaa0 > src/java/main/org/apache/zookeeper/server/DataTree.java 757a572 > src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java > 336827a > src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 1a80d74 > src/java/main/org/apache/zookeeper/server/Request.java c6a2249 > src/java/main/org/apache/zookeeper/server/WatchManager.java 0e7c815 > src/java/main/org/apache/zookeeper/server/ZKDatabase.java 2842803 > src/java/test/org/apache/zookeeper/test/RemoveWatchesTest.java PRE-CREATION > src/zookeeper.jute d24e145 > > Diff: https://reviews.apache.org/r/3364/diff > > > Testing > ------- > > Added unit test that checks client side semantics. > > > Thanks, > > Daniel > >
