You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by "kezhuw (via GitHub)" <gi...@apache.org> on 2023/06/10 11:31:44 UTC
[GitHub] [zookeeper] kezhuw opened a new pull request, #2006: Zookeeper 4472 remove persistent watchers individually
kezhuw opened a new pull request, #2006:
URL: https://github.com/apache/zookeeper/pull/2006
[ZOOKEEPER-4466][] supports different watch modes one same path, but there are no corresponding `WatcherType`s for persistent watches. Client has to resort to `WatcherType.Any` to remove them. This could accidently interrupt other watches.
This PR adds `WatcherType.Persistent` and `WatcherType.PersistentRecursive` to remove persistent watches individually.
Besides above, this pr bases on #1998.
[ZOOKEEPER-4466]: https://issues.apache.org/jira/browse/ZOOKEEPER-4466
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tisonkun commented on pull request #2006: ZOOKEEPER-4472: Remove persistent watches individually
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #2006:
URL: https://github.com/apache/zookeeper/pull/2006#issuecomment-1592374035
Merging...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tisonkun commented on a diff in pull request #2006: ZOOKEEPER-4472: Remove persistent watches individually
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #2006:
URL: https://github.com/apache/zookeeper/pull/2006#discussion_r1228402918
##########
zookeeper-server/src/main/java/org/apache/zookeeper/ZKWatchManager.java:
##########
@@ -225,18 +237,6 @@ void containsWatcher(String path, Watcher watcher, Watcher.WatcherType watcherTy
synchronized (childWatches) {
containsWatcher = contains(path, watcher, childWatches);
}
-
Review Comment:
I agree on this direction that it delivers more reasonable behaviors. But it can somehow still a user-facing manner changes. Maybe we need a release note to describe it.
I'll check the final logic tomorrow and give a review result :D
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw closed pull request #2006: ZOOKEEPER-4472: Remove persistent watches individually
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw closed pull request #2006: ZOOKEEPER-4472: Remove persistent watches individually
URL: https://github.com/apache/zookeeper/pull/2006
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw closed pull request #2006: ZOOKEEPER-4472: Remove persistent watches individually
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw closed pull request #2006: ZOOKEEPER-4472: Remove persistent watches individually
URL: https://github.com/apache/zookeeper/pull/2006
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #2006: ZOOKEEPER-4472: Remove persistent watches individually
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on PR #2006:
URL: https://github.com/apache/zookeeper/pull/2006#issuecomment-1592564736
For the release notes the release manager should write it in the news page.
Tagging @anmolnar who is the RM for 3.9.0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw commented on pull request #2006: ZOOKEEPER-4472: Remove persistent watches individually
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #2006:
URL: https://github.com/apache/zookeeper/pull/2006#issuecomment-1589252656
[ReadOnlyModeTest.testConnectionEvents](https://github.com/apache/zookeeper/actions/runs/5254833209/jobs/9493940619?pr=2006#step:7:1009) failed. There are candidates to fix this #1667 and #1896.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw commented on a diff in pull request #2006: ZOOKEEPER-4472: Remove persistent watches individually
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #2006:
URL: https://github.com/apache/zookeeper/pull/2006#discussion_r1228953716
##########
zookeeper-server/src/main/java/org/apache/zookeeper/ZKWatchManager.java:
##########
@@ -225,18 +237,6 @@ void containsWatcher(String path, Watcher watcher, Watcher.WatcherType watcherTy
synchronized (childWatches) {
containsWatcher = contains(path, watcher, childWatches);
}
-
Review Comment:
I have add "Release Note" to [ZOOKEEPER-4466](https://issues.apache.org/jira/browse/ZOOKEEPER-4466), [ZOOKEEPER-4471](https://issues.apache.org/jira/browse/ZOOKEEPER-4471) and [ZOOKEEPER-4472](https://issues.apache.org/jira/browse/ZOOKEEPER-4472).
Is that sufficient for us ? @eolivelli @tisonkun @anmolnar
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw commented on a diff in pull request #2006: ZOOKEEPER-4472: Remove persistent watches individually
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #2006:
URL: https://github.com/apache/zookeeper/pull/2006#discussion_r1228921954
##########
zookeeper-server/src/main/java/org/apache/zookeeper/ZKWatchManager.java:
##########
@@ -225,18 +237,6 @@ void containsWatcher(String path, Watcher watcher, Watcher.WatcherType watcherTy
synchronized (childWatches) {
containsWatcher = contains(path, watcher, childWatches);
}
-
Review Comment:
Here are my findings:
* `containsWatcher` is package private and used solely by `ZKWatchManager::removeWatcher`.
* `removeWatcher` enforces more stringent `NoWatcherException` check than `containsWatcher`.
So, we are in lucking path that partially removing `WatcherType::Data` from `AddWatchMode::Persistent` is an error even it is permitted by server side(a.k.a `rc` is 0) and `containsWatcher`.
I pushed a test commit for this. I also pushed tests in https://github.com/kezhuw/zookeeper/commit/8c35fcecef65c8f5f41379ffd3e1bcf8744f4551 before ZOOKEEPER-4466. All these tests result in `NOWATCHER` when removing `WatcherType::Data` from `AddWatchMode::Persistent`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tisonkun merged pull request #2006: ZOOKEEPER-4472: Remove persistent watches individually
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun merged PR #2006:
URL: https://github.com/apache/zookeeper/pull/2006
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org