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

LiveTseverSet may not be handling watch removal #5157

Open
keith-turner opened this issue Dec 9, 2024 · 1 comment · May be fixed by #5183
Open

LiveTseverSet may not be handling watch removal #5157

keith-turner opened this issue Dec 9, 2024 · 1 comment · May be fixed by #5183
Labels
blocker This issue blocks any release version labeled on it. bug This issue has been verified to be a bug.
Milestone
4.0.0

Comments

@keith-turner
Copy link
Contributor

Describe the bug

The changes in #5143 introduced new behavior of removing watches in zoocache. ZooCache support passing in external watcher on construction. The only class that seems to utilize this feature is LiveTserverSet. LiveTseverSet may not be handling these changes to remove watches correctly.

Expected behavior

All code that passes a watcher to the zoocache constructor correctly handles watches being removed. More generally may want to evaluate if external code should pass a watcher to zoocache.
@keith-turner keith-turner added blocker This issue blocks any release version labeled on it. bug This issue has been verified to be a bug. labels Dec 9, 2024
@keith-turner keith-turner added this to the 4.0.0 milestone Dec 9, 2024
@dlmarion
Copy link
Contributor

From what I can tell LiveTServerSet implements Watcher so that it can be passed to ZooCache and so ZooCache can call processEvent. The external watcher (LiveTServerSet) is never set on ZooKeeper, it's just using the interface so that it can do the callback. We could change the interface if we wanted to avoid confusion.

dlmarion added a commit to dlmarion/accumulo that referenced this issue Dec 13, 2024
@dlmarion
Reduce number of ZooCache instances and Watchers
5d23c12 
This commit removes most of the places where ZooCache
instances were being created in favor of re-using the
ZooCache from the ClientContext. Additionally, this
commit does not place a Watcher on each node that is
cached and instead places a single persistent
recursive Watcher at the paths in which the caching
is taking place.

This change roughly reduces the Watchers reported in
WatchTheWatchCountIT by 50%. While reducing the number
of Watchers, this commit could reduce ZooKeeper server
performance in two ways:

  1. There is a note in the ZooKeeper javadoc for the
     AddWatchMode enum that states there is a small
     performance decrease when using recursive watchers
     as all of the segments of ZNode paths need to be
     checked for watch triggering.

  2. Because a Watcher is not set on each node this
     commit modified the ZooCache.ZCacheWatcher to
     remove the parent of the triggered node, the
     triggered node, and all of its siblings from the
     cache. This overmatching may mean increased
     lookups in ZooKeeper.

Related to apache#5134
Closes apache#5154, apache#5157
@dlmarion dlmarion linked a pull request Dec 13, 2024 that will close this issue
Open
@dlmarion dlmarion linked a pull request Dec 13, 2024 that will close this issue
Reduce number of ZooCache instances and Watchers #5183
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
blocker This issue blocks any release version labeled on it. bug This issue has been verified to be a bug.
Projects
None yet
Milestone
4.0.0
Development

Successfully merging a pull request may close this issue.

Reduce number of ZooCache instances and Watchers dlmarion/accumulo
2 participants
@dlmarion @keith-turner