ZOOKEEPER-4475: Fix NodeChildrenChanged delivered to recursive watcher by kezhuw · Pull Request #1820 · apache/zookeeper

kezhuw · 2022-02-22T11:23:52Z

Cause:

Events are delivered to recursive watcher unconditionally.
Client could receive NodeChildrenChanged due to child watcher on recursive watching node's descendants.

Changes: Filter out NodeChildrenChanged events for recursive watcher in client side.

kezhuw · 2022-02-25T14:35:17Z

Ping @eolivelli @Randgalt for review.

kezhuw · 2022-03-09T03:15:59Z

Another ping @eolivelli @Randgalt @symat @maoling for review.

kezhuw · 2022-04-06T02:06:02Z

@eolivelli @Randgalt @symat @maoling @arshadmohammad @anmolnar Anyone have time capacity to review this pr ? I think @Randgalt has confirmed this as bug in ZOOKEEPER-4466.

tisonkun

@kezhuw Thanks for your contribution!

Generally looks good. Comments inline.

tisonkun · 2022-06-28T15:56:44Z

+            zk.create("/a/b/c", new byte[0], ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+
+            assertEvent(events, Watcher.Event.EventType.NodeCreated, "/a/b");
+            assertEvent(events, Watcher.Event.EventType.NodeCreated, "/a/b/c");


You may assert no outstanding event here.

Add assertTrue(events.isEmpty()) to assert that no more events in queue.

tisonkun · 2022-06-28T15:57:19Z

        synchronized (persistentWatches) {
            addTo(persistentWatches.get(clientPath), result);
        }
+        if (type == Watcher.Event.EventType.NodeChildrenChanged) {


A comment about reason to filter NodeChildrenChanged event helps - it's not quite straightforward.

Add comments to explain why this is necessary and why this must be done in client side.

tisonkun · 2022-06-28T16:07:07Z

Some other concerns:

The description in this patch is different from JIRA's content:

This requires server to never deliver NodeChildrenChanged for node's descendants.

In this patch we filter event on the client side, which has nothing to do with server side logics.

Do you have a real world workload needs this patch? I'm glad to see a concrete use case for PersistentRecursiveWatch and how the behavior changed here affects the application logic.

kezhuw · 2022-06-29T03:07:32Z

@tisonkun Thank you for reviews. I have pushed fixup commit to resolve your comments. Regarding your concerns, I try to reply them here:

The next sentence after the quoted one is "It can not be true.". From server's perspective, it is valid to attach standard child watches on descendants of node which is being watched in persistent recursive mode. So, basically, server is innocent to this. It has to be solved in client side. I explains this in code comments in fixup commit also.
No. I observed this in writing tests for zookeeper-client-rust. In the first place, I observed quirk behaviors in situations where standard watches and persistent watches are attached on overlapping paths(ZOOKEEPER-4466). After discussion with @Randgalt in ZOOKEEPER-4466, I split out ZOOKEEPER-4475 as a bug report and ZOOKEEPER-4466 remains more like a feature request.

tisonkun

LGTM

eolivelli · 2022-06-30T13:38:28Z

@tisonkun would you have time to test if Curator still works well with this change ? like running all the tests of Curator

eolivelli · 2022-06-30T13:39:19Z

I'm glad to see a concrete use case for PersistentRecursiveWatch
@tisonkun for instance Apache Pulsar relies heavily on PersistentRecursiveWatch since version 2.8.0
you can take a look to Pulsar code

tisonkun · 2022-06-30T15:38:10Z

@eolivelli I'll try to do a local test this week. Ping me if I miss it :)

tisonkun · 2022-06-30T15:56:13Z

@eolivelli since Curator doesn't support ZK 3.7+, I'm trying to pick this commit to 3.6.4-SNAPSHOT and run tests. However, I get a compile error when build with 3.6.4:

[ERROR] Bundle org.apache.zookeeper:zookeeper-jute:jar:3.6.4-SNAPSHOT : The default package '.' is not permitted by the Import-Package syntax.
 This can be caused by compile errors in Eclipse because Eclipse creates
valid class files regardless of compile errors.
The following package(s) import from the default package [org.apache.zookeeper.data, org.apache.zookeeper.proto, org.apache.zookeeper.server.persistence, org.apache.zookeeper.server.quorum, org.apache.zookeeper.txn]

I have no idea whether this failure comes from.

kezhuw · 2022-07-04T00:51:45Z

Hi @eolivelli, I have done a test based on #1859(which is a superset of this pr) and apache/curator#426 on branch-3.7. Follow is the verify progress copied from my shell. From my observation, there are flakes, but not failures.

PS: How the community share test report ? (I have deleted the test report from this reply as it is too long. You could see it from initial edit.)

sonatype-lift · 2022-10-10T01:54:44Z

⚠️ 52 God Classes were detected by Lift in this project. Visit the Lift web console for more details.

The semantics of persistent recursive watch promise no child events on descendant nodes. When there are standard child watches on descendants of node being watches in persistent recursive mode, server will deliver child events to client inevitably. So we have to filter out child events for persistent recursive watches on client side.

kezhuw · 2022-10-17T14:28:14Z

cc @eolivelli @Randgalt @symat @maoling @arshadmohammad @anmolnar for reviews.

kezhuw · 2023-02-22T01:19:54Z

cc @eolivelli @Randgalt @symat @maoling @arshadmohammad @anmolnar @ztzg for reviews.

tisonkun

We now have zookeeper-compatibility-tests-curator and I think it's OK to merge now.

tisonkun · 2023-02-22T09:15:06Z

Pending to merge if no more objection this week.

eolivelli · 2023-02-22T09:21:13Z

@tisonkun in ZooKeeper we do not allow to merge this kind of changes if there are not at least 2 sponsors.

Unfortunately currently there are not many active reviewers, but please always wait for a second approval before merging non trivial patches

tisonkun · 2023-02-22T09:22:12Z

@eolivelli OK.

eolivelli

LGTM

This is kind of a breaking change and we should deliver it only on 3.9.0 and not cherry-pick to 3.8.x

tisonkun · 2023-02-22T09:35:35Z

@eolivelli I agree that it changes the event delivered and am OK to deliver it on 3.9.0, while it can be regarded as a bug fix since in 3.6.0 release notes we write:

Note that NodeChildrenChanged events are not triggered for persistent recursive watches as it would be redundant.

kezhuw force-pushed the ZOOKEEPER-4475-fix-recursive-watcher-NodeChildrenChanged branch from 4751fab to de934cc Compare April 2, 2022 12:12

tisonkun reviewed Jun 28, 2022

View reviewed changes

tisonkun approved these changes Jun 29, 2022

View reviewed changes

kezhuw force-pushed the ZOOKEEPER-4475-fix-recursive-watcher-NodeChildrenChanged branch from f7bb53e to 1c5e5ee Compare June 29, 2022 14:31

kezhuw force-pushed the ZOOKEEPER-4475-fix-recursive-watcher-NodeChildrenChanged branch from 1c5e5ee to 3aac293 Compare July 7, 2022 08:48

kezhuw force-pushed the ZOOKEEPER-4475-fix-recursive-watcher-NodeChildrenChanged branch from 3aac293 to 602ff1d Compare July 20, 2022 00:07

kezhuw force-pushed the ZOOKEEPER-4475-fix-recursive-watcher-NodeChildrenChanged branch from 602ff1d to e33fddf Compare October 10, 2022 01:43

kezhuw force-pushed the ZOOKEEPER-4475-fix-recursive-watcher-NodeChildrenChanged branch from e33fddf to ff4750b Compare October 10, 2022 13:28

tisonkun approved these changes Feb 22, 2023

View reviewed changes

tisonkun requested review from eolivelli and symat February 22, 2023 09:14

eolivelli approved these changes Feb 22, 2023

View reviewed changes

eolivelli merged commit 255b0c9 into apache:master Feb 22, 2023

tisonkun mentioned this pull request Feb 28, 2023

ZOOKEEPER-4466: Support different watch modes on same path #1859

Merged

kezhuw mentioned this pull request May 4, 2023

ZOOKEEPER-4541 Ephemeral znode owned by closed session visible in 1 of 3 servers #1925

Closed

kezhuw deleted the ZOOKEEPER-4475-fix-recursive-watcher-NodeChildrenChanged branch September 24, 2023 15:10

kezhuw mentioned this pull request Sep 10, 2024

add test case to test no child event for watcher #1881

Closed

Uh oh!

Conversation

kezhuw commented Feb 22, 2022

Uh oh!

kezhuw commented Feb 25, 2022

Uh oh!

kezhuw commented Mar 9, 2022

Uh oh!

kezhuw commented Apr 6, 2022

Uh oh!

tisonkun left a comment

Choose a reason for hiding this comment

Uh oh!

tisonkun Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

kezhuw Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

tisonkun Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

kezhuw Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

tisonkun commented Jun 28, 2022

Uh oh!

kezhuw commented Jun 29, 2022

Uh oh!

tisonkun left a comment

Choose a reason for hiding this comment

Uh oh!

eolivelli commented Jun 30, 2022

Uh oh!

eolivelli commented Jun 30, 2022

Uh oh!

tisonkun commented Jun 30, 2022

Uh oh!

tisonkun commented Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kezhuw commented Jul 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonatype-lift Bot commented Oct 10, 2022

Uh oh!

kezhuw commented Oct 17, 2022

Uh oh!

kezhuw commented Feb 22, 2023

Uh oh!

tisonkun left a comment

Choose a reason for hiding this comment

Uh oh!

tisonkun commented Feb 22, 2023

Uh oh!

eolivelli commented Feb 22, 2023

Uh oh!

tisonkun commented Feb 22, 2023

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

tisonkun commented Feb 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tisonkun commented Jun 30, 2022 •

edited

Loading

kezhuw commented Jul 4, 2022 •

edited

Loading