Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 4 commits into from
May 5, 2023
Merged

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

merged 4 commits into from
May 5, 2023

Conversation

kezhuw
Copy link
Member

@kezhuw kezhuw commented Apr 9, 2022

Currently, in server side, there is only one watcher mode per path. This
will destroy and corrupt old watch when client watch a watching path.

eolivelli
eolivelli requested changes Apr 9, 2022
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great work

@lvfangmin @Randgalt @breed please take a look carefully.

I am afraid that we could inadvertently change the semantics of watchers

zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatchManager.java Outdated Show resolved Hide resolved
zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatchStats.java Outdated Show resolved Hide resolved
zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatchStats.java Outdated Show resolved Hide resolved
@eolivelli
Copy link
Contributor

We need more eyes on this patch

@kezhuw kezhuw force-pushed the ZOOKEEPER-4466-different-watch-modes-coexist-with-same-path branch 2 times, most recently from 1722faa to c8557af Compare July 1, 2022 03:24
@kezhuw kezhuw mentioned this pull request Jul 4, 2022
Merged
@kezhuw kezhuw force-pushed the ZOOKEEPER-4466-different-watch-modes-coexist-with-same-path branch from c8557af to 4bdd355 Compare February 23, 2023 04:08
@kezhuw
ZOOKEEPER-4466: Support different watch modes on same path
b65852e 
Currently, in server side, there is only one watcher mode per path. This
will destroy and corrupt old watch when client watch a watching path.
@kezhuw kezhuw force-pushed the ZOOKEEPER-4466-different-watch-modes-coexist-with-same-path branch from 4bdd355 to b65852e Compare February 27, 2023 04:12
@kezhuw
Copy link
Member Author

kezhuw commented Feb 27, 2023

Could you please take a look at this ? @tisonkun @eolivelli @Randgalt @symat @maoling @arshadmohammad @anmolnar @ztzg @lvfangmin @breed

@tisonkun tisonkun self-requested a review February 27, 2023 14:31
eolivelli
eolivelli approved these changes Feb 27, 2023
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Unfortunately we need more eyes on this patch.

@tisonkun
refactor WatchStats internals
cc74acd 
Signed-off-by: tison <wander4096@gmail.com>
tisonkun
tisonkun reviewed Mar 4, 2023
View reviewed changes
zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatchManager.java
}
}
return false;
Set<Watcher> list = watchTable.get(path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kezhuw Can you explain a bit why this change is correct?

It seems that previously, containsWatcher (path, watcher) will be true if any parent of the path has a persistent recursive watch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that previously, containsWatcher (path, watcher) will be true if any parent of the path has a persistent recursive watch.

It is no true though it might intend to do so. Two cases for old code:

  • If there is a watch for (watcher, path), the first return true will hold.
  • If there is no watch for (watcher, path), watcherMode will be WatcherMode.STANDARD, no return true will hold:
    • First return true will not hold as it only apply for leaf node where there is no such watch.
    • Second return true will not hold as WatcherMode.STANDARD is no recursive.

So my change has same behavior as old code and also same to when it was introduced. I think this behavior meets what OpCode.checkWatches expects which is the sole client visible call to this method. Disregard cheating behaviour of ZooKeeper.removeWatches, removing sub nodes from recursive watching sounds quirk as similar one I saw in ZOOKEEPER-4471.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your explanation! I get it now :)

tisonkun
tisonkun reviewed Mar 4, 2023
View reviewed changes
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest looks good to me. One comment above.

@tisonkun
try fix CI
5355029 
Signed-off-by: tison <wander4096@gmail.com>
tisonkun
tisonkun approved these changes Mar 4, 2023
View reviewed changes
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatchManager.java
}
}
return false;
Set<Watcher> list = watchTable.get(path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your explanation! I get it now :)

@tisonkun
Copy link
Member

tisonkun commented Mar 4, 2023

Cc @Randgalt @lvfangmin @breed if you can take a look also.

@kezhuw
Copy link
Member Author

kezhuw commented Apr 27, 2023

Shall we merge this ? Or we need more eyeballs on this? @eolivelli @tisonkun @Randgalt @lvfangmin @breed @cnauroth @symat @maoling

I guess lazy consensus could apply here if no more comments.

@kezhuw kezhuw mentioned this pull request May 4, 2023
Closed
@tisonkun
Copy link
Member

tisonkun commented May 5, 2023

Merging...

Thanks for your contribution @kezhuw!

@tisonkun tisonkun merged commit a64dbf5 into apache:master May 5, 2023
@kezhuw kezhuw mentioned this pull request May 8, 2023
Merged
kezhuw added a commit to kezhuw/zookeeper that referenced this pull request Jun 16, 2023
@kezhuw
hotfix: Fix testSamePathWithDifferentWatchModes compilation failure
8ded4fc 
PR apache#1950(ZOOKEEPER-4655) was created before apache#1859(ZOOKEEPER-4466) merged.
It changes `assertEvent`'s signature which is depended by apache#1859.
@kezhuw kezhuw mentioned this pull request Jun 16, 2023
Merged
anmolnar pushed a commit that referenced this pull request Jun 16, 2023
@kezhuw
hotfix: Fix testSamePathWithDifferentWatchModes compilation failure (#…
ca89286 
…2012)

PR #1950(ZOOKEEPER-4655) was created before #1859(ZOOKEEPER-4466) merged.
It changes `assertEvent`'s signature which is depended by #1859.
anmolnar pushed a commit that referenced this pull request Jun 16, 2023
@kezhuw @anmolnar
ZOOKEEPER-4655: [ADDENDUM] fix build error (#2012)
6254f33 
PR #1950(ZOOKEEPER-4655) was created before #1859(ZOOKEEPER-4466) merged.
It changes `assertEvent`'s signature which is depended by #1859.
anurag-harness pushed a commit to anurag-harness/zookeeper that referenced this pull request Aug 31, 2023
@kezhuw @tisonkun @anurag-harness
ZOOKEEPER-4466: Support different watch modes on same path (apache#1859)
ef711d1 
Signed-off-by: Kezhu Wang <kezhuw@gmail.com>
Co-authored-by: tison <wander4096@gmail.com>
anurag-harness added a commit to anurag-harness/zookeeper that referenced this pull request Aug 31, 2023
@anurag-harness @kezhuw @tisonkun
ZOOKEEPER-4466: Support different watch modes on same path (apache#1859…
4bfb3d7 
…) (#49)

Signed-off-by: Kezhu Wang <kezhuw@gmail.com>
Co-authored-by: Kezhu Wang <kezhuw@gmail.com>
Co-authored-by: tison <wander4096@gmail.com>
@anurag-harness anurag-harness mentioned this pull request Aug 31, 2023
Merged
anurag-harness pushed a commit to anurag-harness/zookeeper that referenced this pull request Aug 31, 2023
@kezhuw @anurag-harness
ZOOKEEPER-4655: [ADDENDUM] fix build error (apache#2012)
a928ba2 
PR apache#1950(ZOOKEEPER-4655) was created before apache#1859(ZOOKEEPER-4466) merged.
It changes `assertEvent`'s signature which is depended by apache#1859.
anurag-harness added a commit to anurag-harness/zookeeper that referenced this pull request Aug 31, 2023
@anurag-harness @kezhuw
ZOOKEEPER-4655: [ADDENDUM] fix build error (apache#2012) (#56)
3558535 
PR apache#1950(ZOOKEEPER-4655) was created before apache#1859(ZOOKEEPER-4466) merged.
It changes `assertEvent`'s signature which is depended by apache#1859.

Co-authored-by: Kezhu Wang <kezhuw@gmail.com>
rahulrane50 pushed a commit to rahulrane50/zookeeper that referenced this pull request Sep 15, 2023
@kezhuw
ZOOKEEPER-4655: [ADDENDUM] fix build error (apache#2012)
e78d61f 
PR apache#1950(ZOOKEEPER-4655) was created before apache#1859(ZOOKEEPER-4466) merged.
It changes `assertEvent`'s signature which is depended by apache#1859.
@kezhuw kezhuw deleted the ZOOKEEPER-4466-different-watch-modes-coexist-with-same-path branch September 24, 2023 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sonatype-lift sonatype-lift sonatype-lift left review comments

@eolivelli eolivelli eolivelli approved these changes

@tisonkun tisonkun tisonkun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kezhuw @eolivelli @tisonkun