-
Notifications
You must be signed in to change notification settings - Fork 2.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
Update to Sarama 1.20.0 #1248
Update to Sarama 1.20.0 #1248
Conversation
…y ingester Signed-off-by: Chodor Marek <marek.chodor@grupawp.pl>
a7db4ca
to
a34acac
Compare
Codecov Report
@@ Coverage Diff @@
## master #1248 +/- ##
======================================
Coverage 100% 100%
======================================
Files 161 161
Lines 7206 7206
======================================
Hits 7206 7206 Continue to review full report at Codecov.
|
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.
LGTM - Allow us some time to deploy a build of this internally before merging
…msg_format Signed-off-by: Chodor Marek <marek.chodor@grupawp.pl>
@vprithvi how did your internal test go? |
@marqc haven't yet tried it out because of code freeze. Hoping to have a response by next week. |
@objectiser Changes merges with master without conflicts. This branch will always be out-of-date with master because there are multiple commits pushed to master everyday. |
@marqc sorry didn't look at the message properly, thought it was complaining about merge conflict :) |
Last I check Prithvi was about to deploy one instance with the upgraded sarama, I'll check with him tomorrow |
We deployed and saw no significant changes in CPU/Memory/goroutines/etc. Nothing scientific, but good enough to merge! Apologies for the delays. |
Which problem is this PR solving?
Issue #1247
Short description of the changes
Upgrade shopify/Sarama dependency.
I tested ingester app against Kafka server 2.0.1 with log.message.format.version=2.0-IV1