-
Notifications
You must be signed in to change notification settings - Fork 19
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
Fix BackbeatProducer event.error handlers set twice. #2535
Fix BackbeatProducer event.error handlers set twice. #2535
Conversation
Hello benzekrimaha,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
cced960
to
8ce8889
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
... and 1 file with indirect coverage changes
@@ Coverage Diff @@
## development/8.7 #2535 +/- ##
===================================================
+ Coverage 69.37% 69.40% +0.02%
===================================================
Files 194 194
Lines 12792 12792
===================================================
+ Hits 8875 8878 +3
+ Misses 3907 3904 -3
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. |
8ce8889
to
90a5547
Compare
@@ -36,7 +36,6 @@ class BackbeatProducer extends EventEmitter { | |||
this._producer.on('event', event => this._log.info('rdkafka.event', { event })); | |||
this._producer.on('event.log', log => this._log.info('rdkafka.log', { log })); | |||
this._producer.on('warning', warning => this._log.warn('rdkafka.warning', { warning })); | |||
this._producer.on('event.error', err => this._log.error('rdkafka.error', { err })); | |||
this._producer.on('event.throttle', throttle => this._log.info('rdkafka.throttle', { throttle })); |
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.
i think this is more subtle than this.
we set 2 event handlers (which is fine) : this one for logs, always ; the other one to emit an error (but also log) only from a certain point in the state machine
→ we propably want to avoid the duplicate logs, but we still want to have the logs all the time ; without changing the current logic of the automaton...
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.
Went for a flag to avoid duplicate logs here: 82315d2
61af658
to
f4b799a
Compare
f4b799a
to
92520d5
Compare
/approve |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue BB-523. Goodbye benzekrimaha. The following options are set: approve |
Issue : BB-523