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.

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

feat(recipe): add ExistingDataWatch class #648

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jeblair
Copy link
Contributor

@jeblair jeblair commented Jul 2, 2021

This adds a subclass of DataWatch which only operates on existing
ZNodes. If a user uses a DataWatch on a path and the ZNode at that
path is deleted, the DataWatch will still issue an "exists" call
and set a watch right before the final callback. That means that
regardless of the return value of the callback and whether or not
Kazoo will invoke the callback again, the ZooKeeper server still
has a watch entry for that path.

In short, using a DataWatch on a path and then deleting that path
can leak watch entries on the ZooKeeper server.

Because the DataWatch recipe is designed to watch non-existing paths,
this behavior may be desired and relied on by some users, so it's
not considered a bug. But other users may want to use DataWatches
for nodes where this behavior would be a problem.

The ExistingDataWatch class behaves similarly to its parent class,
DataWatch, but it does not set a watch on paths which do not exist
(whether that's because they never existed or were recently deleted).
This means that a user of an ExistingDataWatch can be assured that
after the callback with the deleted event, the watch is removed from
the server.

jeffwidman
jeffwidman previously approved these changes Aug 3, 2021
Copy link
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Thank you for including tests.

@ceache
Copy link
Contributor

ceache commented Jun 2, 2022

Resurrecting this PR conversation a little bit.

I am just wondering in which case one would use DataWatch over ExistingDataWatch?
The leak issue rational is clear in the comment above, but what do you lose by removing that watch?

In short, I am wondering why we would want a different class instead of putting the fix in Datawatch proper.

This adds a subclass of DataWatch which only operates on existing
ZNodes.  If a user uses a DataWatch on a path and the ZNode at that
path is deleted, the DataWatch will still issue an "exists" call
and set a watch right before the final callback.  That means that
regardless of the return value of the callback and whether or not
Kazoo will invoke the callback again, the ZooKeeper server still
has a watch entry for that path.

In short, using a DataWatch on a path and then deleting that path
can leak watch entries on the ZooKeeper server.

Because the DataWatch recipe is designed to watch non-existing paths,
this behavior may be desired and relied on by some users, so it's
not considered a bug.  But other users may want to use DataWatches
for nodes where this behavior would be a problem.

The ExistingDataWatch class behaves similarly to its parent class,
DataWatch, but it does not set a watch on paths which do not exist
(whether that's because they never existed or were recently deleted).
This means that a user of an ExistingDataWatch can be assured that
after the callback with the deleted event, the watch is removed from
the server.
@codecov-commenter
Copy link

codecov-commenter commented Jun 2, 2022

Codecov Report

Merging #648 (48f5412) into master (9bb8499) will decrease coverage by 0.43%.
The diff coverage is 98.70%.

@@            Coverage Diff             @@
##           master     #648      +/-   ##
==========================================
- Coverage   94.47%   94.03%   -0.44%     
==========================================
  Files          57       57              
  Lines        8375     8451      +76     
==========================================
+ Hits         7912     7947      +35     
- Misses        463      504      +41     
Impacted Files Coverage Δ
kazoo/recipe/watchers.py 93.75% <95.00%> (+0.14%) ⬆️
kazoo/client.py 97.93% <100.00%> (-0.48%) ⬇️
kazoo/tests/test_watchers.py 99.14% <100.00%> (+0.11%) ⬆️
kazoo/tests/test_cache.py 54.42% <0.00%> (-3.75%) ⬇️
kazoo/protocol/connection.py 92.37% <0.00%> (-2.69%) ⬇️
kazoo/testing/harness.py 95.83% <0.00%> (-2.50%) ⬇️
kazoo/handlers/gevent.py 92.47% <0.00%> (-2.16%) ⬇️
kazoo/tests/test_eventlet_handler.py 87.97% <0.00%> (-1.64%) ⬇️
kazoo/tests/util.py 62.50% <0.00%> (-1.39%) ⬇️
kazoo/handlers/eventlet.py 99.01% <0.00%> (-0.99%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9bb8499...48f5412. Read the comment docs.

@jeblair
Copy link
Contributor Author

jeblair commented Jun 2, 2022

A possible use case for DataWatch is to wait for a znode to appear when none is present already. I believe I wasn't able to see a way to eliminate the leak when a znode is removed while also supporting that behavior. I'm fuzzy on the details (it's been a while!) but I think that's the key assumption I was working from.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants