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.

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

Merged
merged 5 commits into from
Jan 17, 2019
Merged

Conversation

marqc
Copy link
Contributor

@marqc marqc commented Dec 13, 2018

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

@codecov
Copy link

codecov bot commented Dec 13, 2018

Codecov Report

Merging #1248 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1248   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         161     161           
  Lines        7206    7206           
======================================
  Hits         7206    7206

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d64cb70...7440534. Read the comment docs.

@vprithvi vprithvi changed the title Sarama msg format Update to Sarama 1.20.0 Dec 13, 2018
Copy link
Contributor

@vprithvi vprithvi left a 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

@marqc
Copy link
Contributor Author

marqc commented Jan 2, 2019

@vprithvi how did your internal test go?

@vprithvi
Copy link
Contributor

vprithvi commented Jan 2, 2019

@marqc haven't yet tried it out because of code freeze. Hoping to have a response by next week.

@objectiser
Copy link
Contributor

@marqc Could you rebase as the branch is now out of date?

@vprithvi Any more news on the internal tests? Would be good if we can get this in for 1.9.

@marqc
Copy link
Contributor Author

marqc commented Jan 16, 2019

@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.

@objectiser
Copy link
Contributor

@marqc sorry didn't look at the message properly, thought it was complaining about merge conflict :)

@yurishkuro
Copy link
Member

Last I check Prithvi was about to deploy one instance with the upgraded sarama, I'll check with him tomorrow

@ghost ghost assigned vprithvi Jan 17, 2019
@ghost ghost added the review label Jan 17, 2019
@vprithvi
Copy link
Contributor

We deployed and saw no significant changes in CPU/Memory/goroutines/etc. Nothing scientific, but good enough to merge! Apologies for the delays.

@vprithvi vprithvi merged commit 3a3a5e0 into jaegertracing:master Jan 17, 2019
@ghost ghost removed the review label Jan 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants