You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Rakesh R <ra...@huawei.com> on 2014/04/17 14:45:23 UTC
Review Request 20448: ZOOKEEPER-1910: RemoveWatches wrongly removes the
watcher if multiple watches exists on a path
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20448/
-----------------------------------------------------------
Review request for zookeeper, michim and Raul Gutierrez Segales.
Bugs: ZOOKEEPER-1910
https://issues.apache.org/jira/browse/ZOOKEEPER-1910
Repository: zookeeper
Description
-------
RemoveWatches wrongly removes the watcher if multiple watches exists on a path. As per the comments, we have splitted the removeWatches api into:
- removeWatches(path, watcher, watcherType, local);
- removeAllWatches(path, watcherType, local);
Diffs
-----
./src/java/main/org/apache/zookeeper/ZooKeeper.java 1588229
./src/java/main/org/apache/zookeeper/cli/RemoveWatchesCommand.java 1588229
./src/java/main/org/apache/zookeeper/server/DataTree.java 1588229
./src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 1588229
./src/java/main/org/apache/zookeeper/server/WatchManager.java 1588229
./src/java/main/org/apache/zookeeper/server/ZKDatabase.java 1588229
./src/java/test/org/apache/zookeeper/RemoveWatchesTest.java 1588229
./src/zookeeper.jute 1588229
Diff: https://reviews.apache.org/r/20448/diff/
Testing
-------
included tests
Thanks,
Rakesh R
Re: Review Request 20448: ZOOKEEPER-1910: RemoveWatches wrongly removes the
watcher if multiple watches exists on a path
Posted by Raul Gutierrez Segales <rg...@itevenworks.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20448/#review40998
-----------------------------------------------------------
./src/java/main/org/apache/zookeeper/ZooKeeper.java
<https://reviews.apache.org/r/20448/#comment74352>
typo: *registered.
./src/java/main/org/apache/zookeeper/ZooKeeper.java
<https://reviews.apache.org/r/20448/#comment74353>
"whether watches can be removed..."
- Raul Gutierrez Segales
On April 17, 2014, 12:45 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20448/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 12:45 p.m.)
>
>
> Review request for zookeeper, michim and Raul Gutierrez Segales.
>
>
> Bugs: ZOOKEEPER-1910
> https://issues.apache.org/jira/browse/ZOOKEEPER-1910
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> RemoveWatches wrongly removes the watcher if multiple watches exists on a path. As per the comments, we have splitted the removeWatches api into:
> - removeWatches(path, watcher, watcherType, local);
> - removeAllWatches(path, watcherType, local);
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/ZooKeeper.java 1588229
> ./src/java/main/org/apache/zookeeper/cli/RemoveWatchesCommand.java 1588229
> ./src/java/main/org/apache/zookeeper/server/DataTree.java 1588229
> ./src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 1588229
> ./src/java/main/org/apache/zookeeper/server/WatchManager.java 1588229
> ./src/java/main/org/apache/zookeeper/server/ZKDatabase.java 1588229
> ./src/java/test/org/apache/zookeeper/RemoveWatchesTest.java 1588229
> ./src/zookeeper.jute 1588229
>
> Diff: https://reviews.apache.org/r/20448/diff/
>
>
> Testing
> -------
>
> included tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 20448: ZOOKEEPER-1910: RemoveWatches wrongly removes the
watcher if multiple watches exists on a path
Posted by Rakesh R <ra...@huawei.com>.
> On April 22, 2014, 5:51 p.m., Camille Fournier wrote:
> > ./src/java/main/org/apache/zookeeper/ZooKeeper.java, line 2346
> > <https://reviews.apache.org/r/20448/diff/1/?file=561578#file561578line2346>
> >
> > need to remove the part of this that says "or null to remove all watchers"
Thanks Camille, Will fix.
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20448/#review41037
-----------------------------------------------------------
On April 17, 2014, 12:45 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20448/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 12:45 p.m.)
>
>
> Review request for zookeeper, michim and Raul Gutierrez Segales.
>
>
> Bugs: ZOOKEEPER-1910
> https://issues.apache.org/jira/browse/ZOOKEEPER-1910
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> RemoveWatches wrongly removes the watcher if multiple watches exists on a path. As per the comments, we have splitted the removeWatches api into:
> - removeWatches(path, watcher, watcherType, local);
> - removeAllWatches(path, watcherType, local);
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/ZooKeeper.java 1588229
> ./src/java/main/org/apache/zookeeper/cli/RemoveWatchesCommand.java 1588229
> ./src/java/main/org/apache/zookeeper/server/DataTree.java 1588229
> ./src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 1588229
> ./src/java/main/org/apache/zookeeper/server/WatchManager.java 1588229
> ./src/java/main/org/apache/zookeeper/server/ZKDatabase.java 1588229
> ./src/java/test/org/apache/zookeeper/RemoveWatchesTest.java 1588229
> ./src/zookeeper.jute 1588229
>
> Diff: https://reviews.apache.org/r/20448/diff/
>
>
> Testing
> -------
>
> included tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 20448: ZOOKEEPER-1910: RemoveWatches wrongly removes the
watcher if multiple watches exists on a path
Posted by Camille Fournier <sk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20448/#review41037
-----------------------------------------------------------
./src/java/main/org/apache/zookeeper/ZooKeeper.java
<https://reviews.apache.org/r/20448/#comment74390>
need to remove the part of this that says "or null to remove all watchers"
- Camille Fournier
On April 17, 2014, 12:45 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20448/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 12:45 p.m.)
>
>
> Review request for zookeeper, michim and Raul Gutierrez Segales.
>
>
> Bugs: ZOOKEEPER-1910
> https://issues.apache.org/jira/browse/ZOOKEEPER-1910
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> RemoveWatches wrongly removes the watcher if multiple watches exists on a path. As per the comments, we have splitted the removeWatches api into:
> - removeWatches(path, watcher, watcherType, local);
> - removeAllWatches(path, watcherType, local);
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/ZooKeeper.java 1588229
> ./src/java/main/org/apache/zookeeper/cli/RemoveWatchesCommand.java 1588229
> ./src/java/main/org/apache/zookeeper/server/DataTree.java 1588229
> ./src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 1588229
> ./src/java/main/org/apache/zookeeper/server/WatchManager.java 1588229
> ./src/java/main/org/apache/zookeeper/server/ZKDatabase.java 1588229
> ./src/java/test/org/apache/zookeeper/RemoveWatchesTest.java 1588229
> ./src/zookeeper.jute 1588229
>
> Diff: https://reviews.apache.org/r/20448/diff/
>
>
> Testing
> -------
>
> included tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 20448: ZOOKEEPER-1910: RemoveWatches wrongly removes the
watcher if multiple watches exists on a path
Posted by mi...@cs.stanford.edu.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20448/#review41386
-----------------------------------------------------------
Ship it!
Ship It!
- michim
On April 24, 2014, 11:07 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20448/
> -----------------------------------------------------------
>
> (Updated April 24, 2014, 11:07 a.m.)
>
>
> Review request for zookeeper, michim and Raul Gutierrez Segales.
>
>
> Bugs: ZOOKEEPER-1910
> https://issues.apache.org/jira/browse/ZOOKEEPER-1910
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> RemoveWatches wrongly removes the watcher if multiple watches exists on a path. As per the comments, we have splitted the removeWatches api into:
> - removeWatches(path, watcher, watcherType, local);
> - removeAllWatches(path, watcherType, local);
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/ZooDefs.java 1589795
> ./src/java/main/org/apache/zookeeper/ZooKeeper.java 1589795
> ./src/java/main/org/apache/zookeeper/cli/RemoveWatchesCommand.java 1589795
> ./src/java/main/org/apache/zookeeper/server/DataTree.java 1589795
> ./src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 1589795
> ./src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 1589795
> ./src/java/main/org/apache/zookeeper/server/Request.java 1589795
> ./src/java/main/org/apache/zookeeper/server/WatchManager.java 1589795
> ./src/java/main/org/apache/zookeeper/server/ZKDatabase.java 1589795
> ./src/java/test/org/apache/zookeeper/RemoveWatchesTest.java 1589795
> ./src/zookeeper.jute 1589795
>
> Diff: https://reviews.apache.org/r/20448/diff/
>
>
> Testing
> -------
>
> included tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 20448: ZOOKEEPER-1910: RemoveWatches wrongly removes the
watcher if multiple watches exists on a path
Posted by Rakesh R <ra...@huawei.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20448/
-----------------------------------------------------------
(Updated April 24, 2014, 6:07 p.m.)
Review request for zookeeper, michim and Raul Gutierrez Segales.
Changes
-------
Updated new patch addressing the comments. Thanks everyone for your time and reviews
Bugs: ZOOKEEPER-1910
https://issues.apache.org/jira/browse/ZOOKEEPER-1910
Repository: zookeeper
Description
-------
RemoveWatches wrongly removes the watcher if multiple watches exists on a path. As per the comments, we have splitted the removeWatches api into:
- removeWatches(path, watcher, watcherType, local);
- removeAllWatches(path, watcherType, local);
Diffs (updated)
-----
./src/java/main/org/apache/zookeeper/ZooDefs.java 1589795
./src/java/main/org/apache/zookeeper/ZooKeeper.java 1589795
./src/java/main/org/apache/zookeeper/cli/RemoveWatchesCommand.java 1589795
./src/java/main/org/apache/zookeeper/server/DataTree.java 1589795
./src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 1589795
./src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 1589795
./src/java/main/org/apache/zookeeper/server/Request.java 1589795
./src/java/main/org/apache/zookeeper/server/WatchManager.java 1589795
./src/java/main/org/apache/zookeeper/server/ZKDatabase.java 1589795
./src/java/test/org/apache/zookeeper/RemoveWatchesTest.java 1589795
./src/zookeeper.jute 1589795
Diff: https://reviews.apache.org/r/20448/diff/
Testing
-------
included tests
Thanks,
Rakesh R
Re: Review Request 20448: ZOOKEEPER-1910: RemoveWatches wrongly removes the
watcher if multiple watches exists on a path
Posted by Raul Gutierrez Segales <rg...@itevenworks.net>.
> On April 21, 2014, 9:37 p.m., michim wrote:
> > The patch looks good overall. Does this change affect the C client?
>
> Rakesh R wrote:
> Thanks Michi for the review. I'll update the patch.
>
> Reg the C client - yes these apis to be included in C client as well. Hi Raul, I hope we have reached to an agreement. Like previously, it would be great if you can kicks in and do the C client changes ?
>
>
Yes - I'll change the C client.
- Raul
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20448/#review40954
-----------------------------------------------------------
On April 17, 2014, 12:45 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20448/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 12:45 p.m.)
>
>
> Review request for zookeeper, michim and Raul Gutierrez Segales.
>
>
> Bugs: ZOOKEEPER-1910
> https://issues.apache.org/jira/browse/ZOOKEEPER-1910
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> RemoveWatches wrongly removes the watcher if multiple watches exists on a path. As per the comments, we have splitted the removeWatches api into:
> - removeWatches(path, watcher, watcherType, local);
> - removeAllWatches(path, watcherType, local);
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/ZooKeeper.java 1588229
> ./src/java/main/org/apache/zookeeper/cli/RemoveWatchesCommand.java 1588229
> ./src/java/main/org/apache/zookeeper/server/DataTree.java 1588229
> ./src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 1588229
> ./src/java/main/org/apache/zookeeper/server/WatchManager.java 1588229
> ./src/java/main/org/apache/zookeeper/server/ZKDatabase.java 1588229
> ./src/java/test/org/apache/zookeeper/RemoveWatchesTest.java 1588229
> ./src/zookeeper.jute 1588229
>
> Diff: https://reviews.apache.org/r/20448/diff/
>
>
> Testing
> -------
>
> included tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 20448: ZOOKEEPER-1910: RemoveWatches wrongly removes the
watcher if multiple watches exists on a path
Posted by Rakesh R <ra...@huawei.com>.
> On April 21, 2014, 9:37 p.m., michim wrote:
> > The patch looks good overall. Does this change affect the C client?
Thanks Michi for the review. I'll update the patch.
Reg the C client - yes these apis to be included in C client as well. Hi Raul, I hope we have reached to an agreement. Like previously, it would be great if you can kicks in and do the C client changes ?
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20448/#review40954
-----------------------------------------------------------
On April 17, 2014, 12:45 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20448/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 12:45 p.m.)
>
>
> Review request for zookeeper, michim and Raul Gutierrez Segales.
>
>
> Bugs: ZOOKEEPER-1910
> https://issues.apache.org/jira/browse/ZOOKEEPER-1910
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> RemoveWatches wrongly removes the watcher if multiple watches exists on a path. As per the comments, we have splitted the removeWatches api into:
> - removeWatches(path, watcher, watcherType, local);
> - removeAllWatches(path, watcherType, local);
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/ZooKeeper.java 1588229
> ./src/java/main/org/apache/zookeeper/cli/RemoveWatchesCommand.java 1588229
> ./src/java/main/org/apache/zookeeper/server/DataTree.java 1588229
> ./src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 1588229
> ./src/java/main/org/apache/zookeeper/server/WatchManager.java 1588229
> ./src/java/main/org/apache/zookeeper/server/ZKDatabase.java 1588229
> ./src/java/test/org/apache/zookeeper/RemoveWatchesTest.java 1588229
> ./src/zookeeper.jute 1588229
>
> Diff: https://reviews.apache.org/r/20448/diff/
>
>
> Testing
> -------
>
> included tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 20448: ZOOKEEPER-1910: RemoveWatches wrongly removes the
watcher if multiple watches exists on a path
Posted by Rakesh R <ra...@huawei.com>.
> On April 21, 2014, 9:37 p.m., michim wrote:
> > The patch looks good overall. Does this change affect the C client?
>
> Rakesh R wrote:
> Thanks Michi for the review. I'll update the patch.
>
> Reg the C client - yes these apis to be included in C client as well. Hi Raul, I hope we have reached to an agreement. Like previously, it would be great if you can kicks in and do the C client changes ?
>
>
>
> Raul Gutierrez Segales wrote:
> Yes - I'll change the C client.
Thanks again Raul. Please raise JIRA for the same, that will remind us:-)
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20448/#review40954
-----------------------------------------------------------
On April 24, 2014, 6:07 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20448/
> -----------------------------------------------------------
>
> (Updated April 24, 2014, 6:07 p.m.)
>
>
> Review request for zookeeper, michim and Raul Gutierrez Segales.
>
>
> Bugs: ZOOKEEPER-1910
> https://issues.apache.org/jira/browse/ZOOKEEPER-1910
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> RemoveWatches wrongly removes the watcher if multiple watches exists on a path. As per the comments, we have splitted the removeWatches api into:
> - removeWatches(path, watcher, watcherType, local);
> - removeAllWatches(path, watcherType, local);
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/ZooDefs.java 1589795
> ./src/java/main/org/apache/zookeeper/ZooKeeper.java 1589795
> ./src/java/main/org/apache/zookeeper/cli/RemoveWatchesCommand.java 1589795
> ./src/java/main/org/apache/zookeeper/server/DataTree.java 1589795
> ./src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 1589795
> ./src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 1589795
> ./src/java/main/org/apache/zookeeper/server/Request.java 1589795
> ./src/java/main/org/apache/zookeeper/server/WatchManager.java 1589795
> ./src/java/main/org/apache/zookeeper/server/ZKDatabase.java 1589795
> ./src/java/test/org/apache/zookeeper/RemoveWatchesTest.java 1589795
> ./src/zookeeper.jute 1589795
>
> Diff: https://reviews.apache.org/r/20448/diff/
>
>
> Testing
> -------
>
> included tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 20448: ZOOKEEPER-1910: RemoveWatches wrongly removes the
watcher if multiple watches exists on a path
Posted by mi...@cs.stanford.edu.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20448/#review40954
-----------------------------------------------------------
The patch looks good overall. Does this change affect the C client?
./src/java/main/org/apache/zookeeper/ZooKeeper.java
<https://reviews.apache.org/r/20448/#comment74276>
trailing space
./src/java/main/org/apache/zookeeper/ZooKeeper.java
<https://reviews.apache.org/r/20448/#comment74280>
Could you refactor the code to avoid duplicating the logic in sync and async versions?
./src/zookeeper.jute
<https://reviews.apache.org/r/20448/#comment74275>
trailing space
- michim
On April 17, 2014, 5:45 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20448/
> -----------------------------------------------------------
>
> (Updated April 17, 2014, 5:45 a.m.)
>
>
> Review request for zookeeper, michim and Raul Gutierrez Segales.
>
>
> Bugs: ZOOKEEPER-1910
> https://issues.apache.org/jira/browse/ZOOKEEPER-1910
>
>
> Repository: zookeeper
>
>
> Description
> -------
>
> RemoveWatches wrongly removes the watcher if multiple watches exists on a path. As per the comments, we have splitted the removeWatches api into:
> - removeWatches(path, watcher, watcherType, local);
> - removeAllWatches(path, watcherType, local);
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/ZooKeeper.java 1588229
> ./src/java/main/org/apache/zookeeper/cli/RemoveWatchesCommand.java 1588229
> ./src/java/main/org/apache/zookeeper/server/DataTree.java 1588229
> ./src/java/main/org/apache/zookeeper/server/FinalRequestProcessor.java 1588229
> ./src/java/main/org/apache/zookeeper/server/WatchManager.java 1588229
> ./src/java/main/org/apache/zookeeper/server/ZKDatabase.java 1588229
> ./src/java/test/org/apache/zookeeper/RemoveWatchesTest.java 1588229
> ./src/zookeeper.jute 1588229
>
> Diff: https://reviews.apache.org/r/20448/diff/
>
>
> Testing
> -------
>
> included tests
>
>
> Thanks,
>
> Rakesh R
>
>