Hi, Now, both ZOOKEEPER-4475[1] and ZOOKEEPER-44662[2] are merged. Thank you Enrico and tison for reviewing.
I have moved on to ZOOKEEPER-4471[3] and ZOOKEEPER[4]. ZOOKEEPER-4471[3] reported that `AddWatchMode.PERSISTENT` could be broke(a.k.a partially removed) by `removeWatches` `WatcherType.Data` or `WatcherType.Children`. I think we should avoid doing such magic/compute things on the server side. ZOOKEEPER-4472[4] proposed to add new `WatcherType`s to remove `AddWatchMode.PERSISTENT` and `AddWatchMode.PERSISTENT_RECUR SIVE` individually. Currently, they can only be removed by `WatcherType.Any`. After ZOOKEEPER-4466, which enables multiple watch modes on one path, we need separated `WatcherType`s to remove them without interfering with others. Look forward to your reviews in pending pr #1998[5] for ZOOKEEPER-4471. [1]: https://issues.apache.org/jira/browse/ZOOKEEPER-4475 [2]: https://issues.apache.org/jira/browse/ZOOKEEPER-4466 [3]: https://issues.apache.org/jira/browse/ZOOKEEPER-4471 [4]: https://issues.apache.org/jira/browse/ZOOKEEPER-4472 [5]: https://github.com/apache/zookeeper/pull/1998 I am sending this reply for the second time, I can't see my reply from the mailing list after 14 hours. Sorry for being boring if you got two copies. Best, Kezhu Wang On Tue, May 9, 2023 at 12:01 AM Kezhu Wang <kez...@gmail.com> wrote: > > Hi, > > Now, both ZOOKEEPER-4475[1] and ZOOKEEPER-44662[2] are merged. Thank > you Enrico and tison for reviewing. > > I have moved on to ZOOKEEPER-4471[3] and ZOOKEEPER[4]. > > ZOOKEEPER-4471[3] reported that `AddWatchMode.PERSISTENT` could be > broke(a.k.a partially removed) by `removeWatches` `WatcherType.Data` > or `WatcherType.Children`. I think we should avoid doing such > magic/compute things on the server side. > > ZOOKEEPER-4472[4] proposed to add new `WatcherType`s to remove > `AddWatchMode.PERSISTENT` and `AddWatchMode.PERSISTENT_RECURSIVE` > individually. Currently, they can only be removed by > `WatcherType.Any`. After ZOOKEEPER-4466, which enables multiple watch > modes on one path, we need separated `WatcherType`s to remove them > without interfering with others. > > Look forward to your reviews in pending pr #1998[5] for ZOOKEEPER-4471. > > [1]: https://issues.apache.org/jira/browse/ZOOKEEPER-4475 > [2]: https://issues.apache.org/jira/browse/ZOOKEEPER-4466 > [3]: https://issues.apache.org/jira/browse/ZOOKEEPER-4471 > [4]: https://issues.apache.org/jira/browse/ZOOKEEPER-4472 > [5]: https://github.com/apache/zookeeper/pull/1998 > > Best, > Kezhu Wang > > On Mon, Feb 27, 2023 at 9:19 PM Kezhu Wang <kez...@gmail.com> wrote: > > > > Hi eolivelli, tison, > > > > Thank you for reviewing and merging ZOOKEEPER-4475[1](pr#1820[2]). > > > > It is time to move on ZOOKEEPER-4466[3](pr#1859[4]) which make standard > > watch and persistent watch orthogonal on same path. > > > > Contrast to pure client side fix ZOOKEEPER-4475[1], ZOOKEEPER-4466[3] > > demands only server side changes. I agree to what @eolivelli says "We need > > more eyes on this patch". Ping here for possible more attentions. > > > > [1]: https://issues.apache.org/jira/browse/ZOOKEEPER-4475 > > [2]: https://github.com/apache/zookeeper/pull/1820 > > [3]: https://issues.apache.org/jira/browse/ZOOKEEPER-4466 > > [4]: https://github.com/apache/zookeeper/pull/1859 > > > > Best, > > Kezhu Wang > > > > On Sat, Dec 24, 2022 at 8:33 PM Enrico Olivelli <eolive...@gmail.com> wrote: > >> > >> Kezhu, > >> Sorry for late reply. > >> We should definitely move forward with this work > >> > >> > >> Enrico > >> > >> Il Lun 17 Ott 2022, 16:27 Kezhu Wang <kez...@gmail.com> ha scritto: > >> > >> > Ping. > >> > > >> > Best, > >> > Kezhu Wang > >> > > >> > > >> > On July 1, 2022 at 11:38:16, Kezhu Wang (kez...@gmail.com) wrote: > >> > > >> > Hi tison, > >> > > >> > Thank you for reviewing. > >> > > >> > pr#1859 tries to support standard watches and persistent watches on same > >> > paths. It has no code conflicts with pr#1820, but test requirement on > >> > pr#1820. Assumes that: > >> > > >> > 1. Persistent watch (and/or child watch) on “/a” > >> > 2. Persistent recursive watch on “/a” > >> > > >> > Ideally, persistent watch and/or child watch should receive > >> > `NodeChildrenChanged` while persistent recursive watch should not. > >> > Without > >> > pr#1820 which filter out `NodeChildrenChanged` for persistent recursive > >> > watch in client side, test introduced in pr#1859 will fail. > >> > > >> > There are other followups, which are related to watcher removing, I have > >> > reported but blocked by pr#1859(aka. ZOOKEEPER-4466): > >> > * ZOOKEEPER-4471[1]: Remove WatcherType.Children break persistent > >> > watcher's > >> > child events > >> > * ZOOKEEPER-4472[2]: Support persistent watchers removing individually > >> > > >> > [1]: https://issues.apache.org/jira/browse/ZOOKEEPER-4471 > >> > [2]: https://issues.apache.org/jira/browse/ZOOKEEPER-4472 > >> > > >> > Best, > >> > Kezhu Wang > >> > > >> > On June 29, 2022 at 17:19:37, tison (wander4...@gmail.com) wrote: > >> > > >> > Thanks for your contribution Kezhu! > >> > > >> > I've reviewed PR-1820. It looks good to me. PR-1859 seems a followup of > >> > 1820, will review 1859 after 1820 get accepted. > >> > > >> > Best, > >> > tison. > >> > > >> > > >> > Kezhu Wang <kez...@gmail.com> 于2022年6月28日周二 23:17写道： > >> > > >> > > Hi guys, > >> > > > >> > > First, let me summarize changes of these two issues and associated prs > >> > > here. > >> > > > >> > > ZOOKEEPER-4475[1] reports that NodeChildrenChanged could be delivered > >> > > to > >> > > persistent recursive watchers if a child watch is created on > >> > > descendants > >> > of > >> > > node being watched using persistent recursive watch. pr#1820[2] solves > >> > this > >> > > by filtering out NodeChildrenChanged events for persistent recursive > >> > > watches on the client side. > >> > > > >> > > ZOOKEEPER-4466[3] reports that standard watch and persistent watch > >> > > could > >> > > not coexist on same path. pr#1859[4] introduces WatchStats to count and > >> > > coexist different modes on same path. > >> > > > >> > > pr#1820 has been opened for a while but received no reviews. I think it > >> > is > >> > > pretty simple and solves a simple bug. It should take a long time to > >> > > review. > >> > > > >> > > For pr#1859, @eolivelli has given valuable comments. But both I and > >> > > @eolivelli think ZOOKEEPER-4466 deserves more attention. So, basically, > >> > we > >> > > need more reviewers to make sure pr#1859 goes in the right direction > >> > > and > >> > > breaks no sensible codes. > >> > > > >> > > It would be appreciated if any reviewers could take a look at these > >> > > prs. > >> > > > >> > > Best, > >> > > Kezhu Wang > >> > > > >> > > [1]: https://issues.apache.org/jira/browse/ZOOKEEPER-4475 > >> > > [2]: https://github.com/apache/zookeeper/pull/1820 > >> > > [3]: https://issues.apache.org/jira/browse/ZOOKEEPER-4466 > >> > > [4]: https://github.com/apache/zookeeper/pull/1859 > >> > > > >> >
