-
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
supports customized kafka client id #1507
supports customized kafka client id #1507
Conversation
Signed-off-by: Peter Deng <[email protected]>
Signed-off-by: Peter Deng <[email protected]>
6357bae
to
0143c36
Compare
Codecov Report
@@ Coverage Diff @@
## master #1507 +/- ##
=========================================
Coverage ? 99.79%
=========================================
Files ? 182
Lines ? 8716
Branches ? 0
=========================================
Hits ? 8698
Misses ? 9
Partials ? 9
Continue to review full report at Codecov.
|
@@ -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" |
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.
nit: this can be private
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.
Thanks. it's updated.
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, apart from minor nit
cmd/ingester/app/flags.go
Outdated
@@ -64,6 +68,7 @@ const ( | |||
// Options stores the configuration options for the Ingester | |||
type Options struct { | |||
kafkaConsumer.Configuration | |||
ClientID string |
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.
I didn't notice this earlier - but we should move this to kafkaConsumer.Configuration
which is where GroupID/Topic/etc are configured.
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.
yes it is redundant. it is removed.
Signed-off-by: Peter Deng <[email protected]>
d2d379d
to
6d7328c
Compare
Thanks for the contribution! |
Which problem is this PR solving?
#1506
Short description of the changes
supports customized kafka client id in ingester