-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 Kafka receiver closing ready channel multiple times #1696
Fix Kafka receiver closing ready channel multiple times #1696
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1696 +/- ##
==========================================
+ Coverage 92.36% 92.38% +0.01%
==========================================
Files 259 259
Lines 18201 18203 +2
==========================================
+ Hits 16812 16816 +4
+ Misses 976 975 -1
+ Partials 413 412 -1
Continue to review full report at Codecov.
|
consumer/pdata/generated_metrics.go
Outdated
@@ -607,8 +607,6 @@ func (ms Metric) SetUnit(v string) { | |||
(*ms.orig).Unit = v | |||
} | |||
|
|||
|
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.
make fmt
seems to touch this file. I can revert if needed.
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.
It shouldn't anymore.
There is a data race in the test |
da70ea7
to
1f5eaa6
Compare
|
||
logger *zap.Logger | ||
} | ||
|
||
var _ sarama.ConsumerGroupHandler = (*consumerGroupHandler)(nil) | ||
|
||
func (c *consumerGroupHandler) Setup(session sarama.ConsumerGroupSession) error { | ||
close(c.ready) | ||
c.readyCloser.Do(func() { |
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.
The setup can be called multiple times
There is an issue with the example, as the Setup method could be called multiple times as part of when a member initially joins a group or a rebalance happens.
The example from sarama https://github.com/Shopify/sarama/blob/master/examples/consumergroup/main.go#L142 reinitializes the ready channel in the consumer loop that leads to data race. For us it's enough to close the ready channel once as it is used only once in receiver start method to block until receiver is ready to consumer mesages.
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
d1046a7
to
b5d88e5
Compare
@bogdandrutu this is ready for review. |
…pen-telemetry#1696) * translatesfx: add support for multiple SA monitors of the same type * translatesfx: respond to PR feedback
Signed-off-by: Pavol Loffay ploffay@redhat.com
Description: <Describe what has changed.
Avoid closing ready channel multiple times.
Link to tracking Issue:
Fixes #1692
Testing: < Describe what testing was performed and which tests were added.>
Documentation: < Describe the documentation added.>