feat: add support for persistent recursive watches #715

jeblair · 2023-03-21T18:00:40Z

Why is this needed?

ZooKeeper 3.6.0 added support for persistent, and persistent recursive watches. This adds the corresponding support to the Kazoo client class.

Does this PR introduce any breaking change?

No breaking changes.

codecov · 2023-03-21T18:40:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.56%. Comparing base (2fb93a8) to head (415dc93).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #715      +/-   ##
==========================================
- Coverage   96.81%   96.56%   -0.26%     
==========================================
  Files          27       27              
  Lines        3549     3639      +90     
==========================================
+ Hits         3436     3514      +78     
- Misses        113      125      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jeblair · 2023-03-22T00:51:57Z

It looks like the test failures are not related to this change.

kazoo/protocol/connection.py

Some changes to Kazoo are needed to support persistent recursive watches. Until those merge upstream, vendor and update the parts of Kazoo we need. Upstream PR: python-zk/kazoo#715 Change-Id: Id6372e4075b5b3ffeeef3e0f4751a71e59001ef9

kt315 · 2024-01-06T15:36:15Z

Hi! Why not merge this?

kt315 · 2024-01-18T14:02:47Z

Hi! Whose approval is need? 🤔

StephenSorriaux · 2024-01-18T14:06:17Z

Hello,

Approval from @python-zk/maintainers is needed. Will try to take a look by end of the week.

Can somebody that already tried the feature can confirm it is working as expected?

jeblair · 2024-01-18T14:24:15Z

Hello,

Approval from @python-zk/maintainers is needed. Will try to take a look by end of the week.

Can somebody that already tried the feature can confirm it is working as expected?

While awaiting review/merge we in the Zuul project have vendored a copy[1] of this and used it successfully in production for about nine months (though it does look like our vendored copy omits the ability to remove watches since we never do so, so we'll have to rely on the tests for that). Thanks for taking a look. :)

[1] https://opendev.org/zuul/nodepool/src/branch/master/nodepool/zk/vendor

StephenSorriaux

Thank you for the PR, the implementation is great, I just added some questions. Would it be possible to take into account the warning from codecov and cover some of the new lines with a test?

kazoo/client.py

kazoo/protocol/connection.py

kazoo/tests/test_client.py

kazoo/client.py

kazoo/tests/test_client.py

kazoo/exceptions.py

ZooKeeper 3.6.0 added support for persistent, and persistent recursive watches. This adds the corresponding support to the Kazoo client class.

StephenSorriaux · 2024-03-16T17:35:36Z

@jeffwidman @ceache @a-ungurianu this one is now ready to be reviewed
@jeblair I made some small changes, mostly based on my previous review, please let me now if you see anything wrong

jeblair · 2024-03-26T14:07:22Z

@jeffwidman @ceache @a-ungurianu this one is now ready to be reviewed @jeblair I made some small changes, mostly based on my previous review, please let me now if you see anything wrong

The changes LGTM, thanks! And sorry I wasn't able to address those myself in a timely manner.

StephenSorriaux · 2024-04-06T17:53:13Z

Thank you @jeblair

@jeffwidman @ceache @a-ungurianu gentle bump!

jeffwidman

Looks good!

I made a few mostly cosmetic suggestions, but functionally it all looks good to me.

There's enough complexity in the protocol that even with the nice unit test coverage it wouldn't surprise me if there was a bug or two in here, either on our end or zookeeper's end, but given the solid UT coverage I think the best way to flush them out at this point is to merge and let folks bang on it for a while. The nice part is that this is a new feature, so any problems are unlikely to affect existing functionality.

jeffwidman · 2024-04-12T14:13:58Z

kazoo/client.py


 class TransactionRequest(object):
+


Is this blank line extraneous? I don't normally recall a blank line between class definition and docstring...

jeffwidman · 2024-04-12T14:19:15Z

kazoo/protocol/connection.py

+                        if request.mode == AddWatchMode.PERSISTENT:
+                            client._persistent_watchers[request.path].add(
+                                watcher
+                            )
+                        elif request.mode == AddWatchMode.PERSISTENT_RECURSIVE:
+                            client._persistent_recursive_watchers[
+                                request.path
+                            ].add(watcher)


I wonder if this might benefit from an else clause?

I realize it's a no-op, but it still took me a moment to realize that's because it might be a non-persistant watch that's being added... so might be easier for a reader to understand with a code comment...

But code comments can go out of sync, so a simple assertion that it's the expected mode might be useful like:

else: if request.mode not in [expected modes]: raise <relevant exception>

that way both clarifies the code and also catches stupid errors if somehow a case was missed down the road.

Alternatively, this might read even simpler as a switch statement with a default case... but I realize those require Python 3.10 so maybe leave a:

# TODO: switch these if/else clauses to switch statements where appropriate once we drop Python 3.9 support

jeffwidman · 2024-04-12T14:20:22Z

kazoo/protocol/connection.py

+                    if request.watcher_type == WatcherType.CHILDREN:
+                        client._child_watchers.pop(request.path, None)
+                    elif request.watcher_type == WatcherType.DATA:
+                        client._data_watchers.pop(request.path, None)
+                    elif request.watcher_type == WatcherType.ANY:


~~this might read simpler as a switch statement rather than the if/elif/elif series? technically it should be more performant too~~

Oh, NVM, just realized switch statements won't work in Python 3.8/3.9 😢

jeffwidman · 2024-04-12T14:25:40Z

kazoo/protocol/states.py

+
+    .. attribute:: PERSISTENT
+
+        The watch is not removed when trigged.


Suggested change

The watch is not removed when trigged.

The watch is not removed when triggered.

jeffwidman · 2024-04-12T14:26:05Z

kazoo/protocol/states.py

+
+    .. attribute:: PERSISTENT_RECURSIVE
+
+        The watch is not removed when trigged, and applies to all


Suggested change

The watch is not removed when trigged, and applies to all

The watch is not removed when triggered, and applies to all

jeffwidman · 2024-04-12T14:43:36Z

kazoo/tests/test_client.py

+            pytest.skip("Must use Zookeeper %s.%s or above" % (major, minor))
+
+    def test_persistent_recursive_watch(self):
+        # This tests adding and removing a persistent recursive watch.


Suggested change

# This tests adding and removing a persistent recursive watch.

"""This tests adding and removing a persistent recursive watch."""

jeffwidman · 2024-04-12T14:45:12Z

kazoo/tests/test_client.py

+        ]
+
+    def test_persistent_watch(self):
+        # This tests adding and removing a persistent watch.


Suggested change

# This tests adding and removing a persistent watch.

"""This tests adding and removing a persistent watch."""

jeffwidman · 2024-04-12T14:46:25Z

kazoo/tests/test_client.py

+            dict(type=EventType.DELETED, path="/a"),
+        ]
+
+    def test_persistent_watch(self):


nit: I'd probably order the tests:

test_persistent_watch

test_persistent_recursive_watch

As 2️⃣ feels like a variant of 1️⃣ , so typically we put the simpler one first.

jeffwidman · 2024-04-12T14:47:22Z

kazoo/tests/test_client.py

+        ]
+
+    def test_remove_data_watch(self):
+        # Test that removing a data watch leaves a child watch in place.


Suggested change

# Test that removing a data watch leaves a child watch in place.

"""Test that removing a data watch leaves a child watch in place."""

jeffwidman · 2024-04-12T14:48:10Z

kazoo/tests/test_client.py

+        callback_event.wait(30)
+
+    def test_remove_children_watch(self):
+        # Test that removing a children watch leaves a data watch in place.


Suggested change

# Test that removing a children watch leaves a data watch in place.

"""Test that removing a children watch leaves a data watch in place."""

jeblair force-pushed the persistent branch from bbf62af to 741f246 Compare March 21, 2023 18:23

westphahl reviewed Mar 22, 2023

View reviewed changes

kazoo/protocol/connection.py Outdated Show resolved Hide resolved

jeblair force-pushed the persistent branch from 741f246 to 34adddf Compare March 22, 2023 15:17

westphahl approved these changes Jan 8, 2024

View reviewed changes

kt315 approved these changes Jan 14, 2024

View reviewed changes

StephenSorriaux force-pushed the persistent branch from 34adddf to 625f7c3 Compare January 30, 2024 02:29

StephenSorriaux reviewed Feb 19, 2024

View reviewed changes

StephenSorriaux mentioned this pull request Mar 8, 2024

Add support for remove_watches operation #233

Open

StephenSorriaux force-pushed the persistent branch 2 times, most recently from 3b030f0 to d2a911e Compare March 16, 2024 15:58

houndci-bot reviewed Mar 16, 2024

View reviewed changes

kazoo/exceptions.py Outdated Show resolved Hide resolved

StephenSorriaux force-pushed the persistent branch from d2a911e to 45cef47 Compare March 16, 2024 16:01

feat(core): add support for persistent recursive watches

415dc93

ZooKeeper 3.6.0 added support for persistent, and persistent recursive watches. This adds the corresponding support to the Kazoo client class.

StephenSorriaux force-pushed the persistent branch from 45cef47 to 415dc93 Compare March 16, 2024 17:18

StephenSorriaux requested review from jeffwidman, ceache and a-ungurianu March 16, 2024 17:34

StephenSorriaux approved these changes Mar 16, 2024

View reviewed changes

jeffwidman approved these changes Apr 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for persistent recursive watches #715

feat: add support for persistent recursive watches #715

jeblair commented Mar 21, 2023

codecov bot commented Mar 21, 2023 •

edited

Loading

jeblair commented Mar 22, 2023

kt315 commented Jan 6, 2024

kt315 commented Jan 18, 2024

StephenSorriaux commented Jan 18, 2024

jeblair commented Jan 18, 2024

StephenSorriaux left a comment

StephenSorriaux commented Mar 16, 2024

jeblair commented Mar 26, 2024

StephenSorriaux commented Apr 6, 2024

jeffwidman left a comment

jeffwidman Apr 12, 2024

jeffwidman Apr 12, 2024

jeffwidman Apr 12, 2024

jeffwidman Apr 12, 2024

jeffwidman Apr 12, 2024

jeffwidman Apr 12, 2024

jeffwidman Apr 12, 2024

jeffwidman Apr 12, 2024

jeffwidman Apr 12, 2024

jeffwidman Apr 12, 2024


		.. attribute:: PERSISTENT

		The watch is not removed when trigged.

	The watch is not removed when trigged.
	The watch is not removed when triggered.


		.. attribute:: PERSISTENT_RECURSIVE

		The watch is not removed when trigged, and applies to all

	The watch is not removed when trigged, and applies to all
	The watch is not removed when triggered, and applies to all

	# This tests adding and removing a persistent recursive watch.
	"""This tests adding and removing a persistent recursive watch."""

	# Test that removing a data watch leaves a child watch in place.
	"""Test that removing a data watch leaves a child watch in place."""

	# Test that removing a children watch leaves a data watch in place.
	"""Test that removing a children watch leaves a data watch in place."""

feat: add support for persistent recursive watches #715

Are you sure you want to change the base?

feat: add support for persistent recursive watches #715

Conversation

jeblair commented Mar 21, 2023

Why is this needed?

Does this PR introduce any breaking change?

codecov bot commented Mar 21, 2023 • edited Loading

Codecov Report

jeblair commented Mar 22, 2023

kt315 commented Jan 6, 2024

kt315 commented Jan 18, 2024

StephenSorriaux commented Jan 18, 2024

jeblair commented Jan 18, 2024

StephenSorriaux left a comment

Choose a reason for hiding this comment

StephenSorriaux commented Mar 16, 2024

jeblair commented Mar 26, 2024

StephenSorriaux commented Apr 6, 2024

jeffwidman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 21, 2023 •

edited

Loading