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

supports customized kafka client id #1507

Conversation

newly12
Copy link
Contributor

@newly12 newly12 commented Apr 29, 2019

Which problem is this PR solving?

#1506

Short description of the changes

supports customized kafka client id in ingester

Signed-off-by: Peter Deng <yundeng@ebay.com>
Signed-off-by: Peter Deng <yundeng@ebay.com>
@newly12 newly12 force-pushed the jaeger_ingester_supports_kafka_client_id branch from 6357bae to 0143c36 Compare April 29, 2019 13:14
@codecov
Copy link

codecov bot commented Apr 29, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@d60b152). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1507   +/-   ##
=========================================
  Coverage          ?   99.79%           
=========================================
  Files             ?      182           
  Lines             ?     8716           
  Branches          ?        0           
=========================================
  Hits              ?     8698           
  Misses            ?        9           
  Partials          ?        9
Impacted Files Coverage Δ
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 d60b152...6d7328c. Read the comment docs.

@@ -46,6 +46,7 @@ func (s *KafkaIntegrationTestSuite) initialize() error {
s.logger, _ = testutils.NewLogger()
const encoding = "json"
const groupID = "kafka-integration-test"
const ClientID = "kafka-integration-test"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this can be private

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. it's updated.

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, apart from minor nit

@@ -64,6 +68,7 @@ const (
// Options stores the configuration options for the Ingester
type Options struct {
kafkaConsumer.Configuration
ClientID string
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't notice this earlier - but we should move this to kafkaConsumer.Configuration which is where GroupID/Topic/etc are configured.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it is redundant. it is removed.

Signed-off-by: Peter Deng <yundeng@ebay.com>
@newly12 newly12 force-pushed the jaeger_ingester_supports_kafka_client_id branch from d2d379d to 6d7328c Compare May 1, 2019 08:25
@vprithvi vprithvi merged commit 15fb6fe into jaegertracing:master May 1, 2019
@vprithvi
Copy link
Contributor

vprithvi commented May 1, 2019

Thanks for the contribution!

@newly12 newly12 deleted the jaeger_ingester_supports_kafka_client_id branch May 2, 2019 13:34
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.

2 participants