Review Board 1.7.22


ZOOKEEPER-442: need a way to remove watches that are no longer of interest

Review Request #3364 - Created Jan. 4, 2012 and updated

Daniel Gómez Ferro
trunk
ZOOKEEPER-442
Reviewers
zookeeper
breed
zookeeper-git
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.
Added unit test that checks client side semantics.
Posted (Jan. 4, 2012, 2:58 p.m.)
This is only a first pass. Looks ok but I would like more attention to anything you're not really sure about.
Maybe collapse this to if(watchers != null && watchers.contains(watcher)) ?
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?
??
Let's not check in files that are just reordering of imports...
Posted (Jan. 4, 2012, 5:40 p.m.)
As part of this patch the programmer guide needs to be updated to detail how this feature works. In particular it should detail the semantics of watch removal, any "gotchas" to watch out for, etc...
Posted (Jan. 4, 2012, 5:58 p.m.)

   

  
This class is getting rather large, it should be refactored into it's own file.
all watchers _registered by this session_, correct?
this is confusing, what does "leak" mean? It might be good to move this detail from here to the programmer guide.
@throws?
Update all the @since with expected release - looks like 3.5.0
why type vs having explicit methods?

removeChildWatches
removeDataWatches

we're unlikely to add more watch types.

On second thought though, shouldn't we have a "both" (or "alltypes", etc...) option as well? If that's the case perhaps 3 is enough to have a type...
since is incorrect
async operations don't throw keeperexception, see other async ops such as getData
see my comment on method signature - your intuition was correct.
Posted (Jan. 4, 2012, 6:16 p.m.)

   

  
is it needed? would be good to be sure before we commit
it would be good to document here why we are calling the async version (that this is the only way to ensure that all pending watches have been processed before notifying that that watch was removed successfully).
ditto