ZOOKEEPER-4471: Match removing WatcherType to standard, persistent modes #1998
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kezhuw
force-pushed
the
ZOOKEEPER-4471-remove-watches
branch
3 times, most recently
from
9b3a2f3
to
287c86e
Compare
|
We are approaching ZOOKEEPER-4472.
Combine them all, we will have orthogonal watcher modes for clients. Watching or un-watching a path in one mode will not break other watching modes on same path. |
Before ZOOKEEPER-1416, `WatcherType.Children` was used to remove watchers attached through `ZooKeeper.getChildren`. `WatcherType.Data` was used to remove watchers attached through `ZooKeeper.getData` and `ZooKeeper.exists`. ZOOKEEPER-1416 adds `AddWatchMode.PERSISTENT`. This watcher could be completed remove using `WatcherType.Any`. But when removing through `WatcherType.Data` or `WatcherType.Children`, part of `AddWatchMode.PERSISTENT` will be left behind. And we get persistent children or data watchers. We are never claiming to support these type of watchers. So fix it. In rare chance, we are going to support persistent data or children watchers in future, I think we probably don't want to do such "magic" thing in ZooKeeper. So fix it. This is a step towards ZOOKEEPER-4472 which proposed to support `WatcherType.Persistent` and `WatcherType.PersistentRecursive` to remove persistent watchers.
kezhuw
force-pushed
the
ZOOKEEPER-4471-remove-watches
branch
from
287c86e
to
33da8ea
Compare
I found it somewhat hard to follow in self-review. Add given-when-then comments from my best hope for reviewing and maintenance.
|
Failure test case run reported at ZOOKEEPER-4512. |
|
Great! Thank you for reviewing! @tisonkun @eolivelli The cpp tests hang, I will reopen this pr for another ci run. |
|
This is the link to previous ci run, the cpp tests hang for almost 5 hours. https://github.com/apache/zookeeper/actions/runs/5251856628/jobs/9487231863 |
|
All checks are green. Can we merge this and move forward to #2006 ? @eolivelli @tisonkun |
|
I think it's good to go. Merging... Thanks for your contribution @kezhuw! |
anurag-harness
pushed a commit
to anurag-harness/zookeeper
that referenced
this pull request
Aug 31, 2023
…des (apache#1998) * ZOOKEEPER-4471: Match removing WatcherType to standard, persistent modes Before ZOOKEEPER-1416, `WatcherType.Children` was used to remove watchers attached through `ZooKeeper.getChildren`. `WatcherType.Data` was used to remove watchers attached through `ZooKeeper.getData` and `ZooKeeper.exists`. ZOOKEEPER-1416 adds `AddWatchMode.PERSISTENT`. This watcher could be completed remove using `WatcherType.Any`. But when removing through `WatcherType.Data` or `WatcherType.Children`, part of `AddWatchMode.PERSISTENT` will be left behind. And we get persistent children or data watchers. We are never claiming to support these type of watchers. So fix it. In rare chance, we are going to support persistent data or children watchers in future, I think we probably don't want to do such "magic" thing in ZooKeeper. So fix it. This is a step towards ZOOKEEPER-4472 which proposed to support `WatcherType.Persistent` and `WatcherType.PersistentRecursive` to remove persistent watchers. * Refactor newly added tests in WatchManagerTest I found it somewhat hard to follow in self-review. Add given-when-then comments from my best hope for reviewing and maintenance.
anurag-harness
added a commit
to anurag-harness/zookeeper
that referenced
this pull request
Aug 31, 2023
…des (apache#1998) (#51) * ZOOKEEPER-4471: Match removing WatcherType to standard, persistent modes Before ZOOKEEPER-1416, `WatcherType.Children` was used to remove watchers attached through `ZooKeeper.getChildren`. `WatcherType.Data` was used to remove watchers attached through `ZooKeeper.getData` and `ZooKeeper.exists`. ZOOKEEPER-1416 adds `AddWatchMode.PERSISTENT`. This watcher could be completed remove using `WatcherType.Any`. But when removing through `WatcherType.Data` or `WatcherType.Children`, part of `AddWatchMode.PERSISTENT` will be left behind. And we get persistent children or data watchers. We are never claiming to support these type of watchers. So fix it. In rare chance, we are going to support persistent data or children watchers in future, I think we probably don't want to do such "magic" thing in ZooKeeper. So fix it. This is a step towards ZOOKEEPER-4472 which proposed to support `WatcherType.Persistent` and `WatcherType.PersistentRecursive` to remove persistent watchers. * Refactor newly added tests in WatchManagerTest I found it somewhat hard to follow in self-review. Add given-when-then comments from my best hope for reviewing and maintenance. Co-authored-by: Kezhu Wang <kezhuw@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before ZOOKEEPER-1416,
WatcherType.Childrenwas used to remove watchers attached throughZooKeeper.getChildren.WatcherType.Datawas used to remove watchers attached throughZooKeeper.getDataandZooKeeper.exists.ZOOKEEPER-1416 adds
AddWatchMode.PERSISTENT. This watcher could be completed remove usingWatcherType.Any. But when removing throughWatcherType.DataorWatcherType.Children, part ofAddWatchMode.PERSISTENTwill be left behind. And we get persistent children or data watchers.We are never claiming to support these type of watchers. So fix it.
In rare chance, we are going to support persistent data or children watchers in future, I think we probably don't want to do such "magic" thing in ZooKeeper. So fix it.
This is a step towards ZOOKEEPER-4472 which proposed to support
WatcherType.PersistentandWatcherType.PersistentRecursiveto remove persistent watchers.