-
Notifications
You must be signed in to change notification settings - Fork 23
BB-727: Fix ZookeeperConfigManager event listener leaks #2687
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
base: development/9.0
Are you sure you want to change the base?
Conversation
Fix `key: { type: 'Buffer', data: [ 104, 101, 108, 108, 111 ] }`
To `key: 'hello'`
The watcher NODE_DELETED already calls removeConfig with false Because it's already removed from zk if the event is received There is no need to send a request to zk
This kind of error can happen if data is undefined when the read is triggered too late when the znode is already removed
Hello bourgoismickael,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 3 files with indirect coverage changes
@@ Coverage Diff @@
## development/9.0 #2687 +/- ##
===================================================
- Coverage 74.27% 74.04% -0.24%
===================================================
Files 201 201
Lines 13444 13458 +14
===================================================
- Hits 9986 9965 -21
- Misses 3448 3483 +35
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
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.
Pull Request Overview
This PR addresses event listener leaks in the ZookeeperConfigManager by improving watcher management and separating list/get config operations. The key improvements include early detection of watcher leaks, preventing redundant ZooKeeper remove operations when watchers notify of deleted bucket configs, and enhanced logging for better debugging.
- Added early warning system for ZooKeeper watcher leaks to detect issues before hitting EventEmitter limits
- Separated
getConfigandlistConfigsevent handlers to avoid unnecessary bucket list operations - Modified
removeConfigto optionally skip ZooKeeper emission when triggered by a watcher notification
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/BackbeatConsumer.js | Enhanced logging to safely convert Buffer keys to strings for better debugging |
| extensions/notification/configManager/ZookeeperConfigManager.js | Core fixes for watcher leak detection, separated list/get config handlers, and added conditional ZK emission for removeConfig |
| extensions/notification/NotificationQueuePopulator.js | Corrected typo in comment from "deleter" to "deleted" |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Otherwise we need a few buckets and bucket operations to trigger the event emitter listener limit warning at 10 listeners
The getConfig should not trigger a listing with watcher as it duplicates an already existing watcher. This can have an exponential effect of adding even more watchers when events are triggered. A clear separation prevent this effect: - only NODE_CHILDREN_CHANGED should trigger a new listing to reapply a watcher - getConfig should be contained to a single bucket (discovered by listing, or from watcher NODE_DATA_CHANGED)
62f6510 to
dc1fe12
Compare
Below a sequence diagram showing in red a few problems fixed here