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.

Sign up for GitHub

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

Jump to bottom

Kafka: Add new Consumer and Producer flags #1360

Merged
merged 3 commits into from
Feb 23, 2019
Merged

Kafka: Add new Consumer and Producer flags #1360

merged 3 commits into from
Feb 23, 2019

Conversation

ledor473
Copy link
Member

@ledor473 ledor473 commented Feb 18, 2019 •
edited
Loading

Which problem is this PR solving?

Short description of the changes

Replace #1231 and implement this: #1231 (comment)

  • Add kafka.producer flags in plugins/storage/kafka
  • Add kafka.consumer flags in Ingester
  • Deprecate kafka flags

@ghost ghost assigned ledor473 Feb 18, 2019
@ghost ghost added the review label Feb 18, 2019
@ledor473 ledor473 changed the title Add new kafka.consumer + kafka.producer flags instead of deprecated f… Kafka: Add new Consumer and Producer flags Feb 18, 2019
yurishkuro
yurishkuro reviewed Feb 19, 2019
View reviewed changes
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a "breaking changes" section to the changelog

cmd/ingester/app/flags.go Outdated Show resolved Hide resolved
cmd/ingester/app/flags.go Outdated
o.Brokers = strings.Split(v.GetString(KafkaConsumerConfigPrefix+SuffixBrokers), ",")
o.Topic = v.GetString(KafkaConsumerConfigPrefix + SuffixTopic)
o.GroupID = v.GetString(KafkaConsumerConfigPrefix + SuffixGroupID)
o.Encoding = v.GetString(KafkaConsumerConfigPrefix + SuffixEncoding)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the new flags are not specified, wouldn't all these values become empty?

Copy link
Member Author

@ledor473 ledor473 Feb 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reworked the PR and added a test for that.
I wasn't sure which flags we should prefer if both are specified, currently the old flags will win over the new flags if both are specified (as per the test)

@ledor473 ledor473 force-pushed the kafka-producer-consumer-flags branch from cae5b63 to f7f0ac3 Compare February 20, 2019 17:00
@ledor473 ledor473 marked this pull request as ready for review February 20, 2019 17:01
@ledor473 ledor473 force-pushed the kafka-producer-consumer-flags branch from f7f0ac3 to e0c9be4 Compare February 20, 2019 17:14
Add new kafka.consumer + kafka.producer flags instead of deprecated f…
468ef51 
…lags

Signed-off-by: Louis-Etienne Dorval <louis-etienne.dorval@ticketmaster.com>
@ledor473 ledor473 force-pushed the kafka-producer-consumer-flags branch from e0c9be4 to 468ef51 Compare February 20, 2019 17:35
@ledor473
Copy link
Member Author

I wonder if retrying the CI jobs would fix it

@ghost ghost assigned yurishkuro Feb 22, 2019
@codecov
Copy link

codecov bot commented Feb 22, 2019 •
edited
Loading

Codecov Report

Merging #1360 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1360   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         164     164           
  Lines        7376    7447   +71     
======================================
+ Hits         7376    7447   +71
Impacted Files Coverage Δ
plugin/storage/kafka/options.go 100% <100%> (ø) ⬆️
cmd/ingester/app/flags.go 100% <100%> (ø) ⬆️

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 2d6b35e...5b47d1b. Read the comment docs.

fmt the new Kafka consumer/producer code
5b47d1b 
Signed-off-by: Louis-Etienne Dorval <louis-etienne.dorval@ticketmaster.com>
@ledor473 ledor473 force-pushed the kafka-producer-consumer-flags branch from 82f14b7 to 5b47d1b Compare February 22, 2019 21:51
yurishkuro
yurishkuro approved these changes Feb 23, 2019
View reviewed changes
cmd/ingester/app/flags.go
o.Topic = v.GetString(KafkaConfigPrefix + SuffixTopic)
o.GroupID = v.GetString(KafkaConfigPrefix + SuffixGroupID)
o.Encoding = v.GetString(KafkaConfigPrefix + SuffixEncoding)
o.Brokers = strings.Split(v.GetString(KafkaConsumerConfigPrefix+SuffixBrokers), ",")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we recently fixed a bug where the list of hosts for Cassandra or ES would not parse correctly if they contained spaces. Perhaps worth reusing the same util method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll bundle that suggestion into the PR tha delete the deprecated flags (to be merged post 1.11)

@yurishkuro yurishkuro merged commit 8f24232 into jaegertracing:master Feb 23, 2019
@ghost ghost removed the review label Feb 23, 2019
@ledor473 ledor473 mentioned this pull request Feb 25, 2019
Closed
konradgaluszka pushed a commit to SabreOSS/jaeger that referenced this pull request Feb 26, 2019
@ledor473
Kafka: Add new Consumer and Producer flags (jaegertracing#1360)
2c8e304 
* Add new kafka.consumer + kafka.producer flags instead of deprecated flags

Signed-off-by: Louis-Etienne Dorval <louis-etienne.dorval@ticketmaster.com>

* fmt the new Kafka consumer/producer code

Signed-off-by: Louis-Etienne Dorval <louis-etienne.dorval@ticketmaster.com>
annanay25 pushed a commit to annanay25/jaeger that referenced this pull request Mar 1, 2019
@ledor473
Kafka: Add new Consumer and Producer flags (jaegertracing#1360)
53fb3bf 
* Add new kafka.consumer + kafka.producer flags instead of deprecated flags

Signed-off-by: Louis-Etienne Dorval <louis-etienne.dorval@ticketmaster.com>

* fmt the new Kafka consumer/producer code

Signed-off-by: Louis-Etienne Dorval <louis-etienne.dorval@ticketmaster.com>
@ledor473 ledor473 mentioned this pull request Mar 15, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@black-adder black-adder Awaiting requested review from black-adder

@jpkrohling jpkrohling Awaiting requested review from jpkrohling

@objectiser objectiser Awaiting requested review from objectiser

@pavolloffay pavolloffay Awaiting requested review from pavolloffay

@tiffon tiffon Awaiting requested review from tiffon

@vprithvi vprithvi Awaiting requested review from vprithvi

Assignees

@ledor473 ledor473

@yurishkuro yurishkuro

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ledor473 @yurishkuro