-
Notifications
You must be signed in to change notification settings - Fork 387
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
Resurrecting this PR conversation a little bit. I am just wondering in which case one would use In short, I am wondering why we would want a different class instead of putting the fix in |
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 Report
@@ 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
Continue to review full report at Codecov.
|
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. |
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.