-
Notifications
You must be signed in to change notification settings - Fork 83
Use sarama config build in source #337
Use sarama config build in source #337
Conversation
93cfbd8
to
f8833d2
Compare
Codecov Report
@@ Coverage Diff @@
## master #337 +/- ##
==========================================
- Coverage 75.27% 72.92% -2.35%
==========================================
Files 124 128 +4
Lines 4792 4975 +183
==========================================
+ Hits 3607 3628 +21
- Misses 962 1114 +152
- Partials 223 233 +10
Continue to review full report at Codecov.
|
d3d4456
to
38b1e0f
Compare
/test pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated |
7ec4112
to
fc6ffc4
Compare
/lgtm /hold waiting for another review |
/assign @travis-minke-sap @eric-sap @matzew |
@aliok: GitHub didn't allow me to assign the following users: eric-sap. Note that only knative-sandbox members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Looks good to me! (I'm not super familiar with the Kafka Source though) Great to see more alignment. I think there might be potential for further consolidation of all the ConfigMapWatchers (and SecretWatchers coming with #373 and #363) but we can address that when they're all in place or maybe during #363 have a look? @eric-sap is going to review this as well - he's a bit more familiar with the Sarama Config handling ; ) |
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.
Nothing problematic from the config handling perspective that I see. Made some minor comments as I was looking.
envs = maybeAppendEnvVar(envs, cw.kafkaConfigEnvVar(), cw.KafkaConfig() != nil) | ||
|
||
return envs | ||
} | ||
|
||
// maybeAppendEnvVar appends an EnvVar only if the condition boolean is true. | ||
func maybeAppendEnvVar(envs []corev1.EnvVar, env corev1.EnvVar, cond bool) []corev1.EnvVar { | ||
if !cond { | ||
return envs | ||
} | ||
|
||
return append(envs, env) | ||
} |
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.
Are there more usages planned for this function? As it is it looks like an overly complicated way of doing this (though nothing technically amiss)
envs = maybeAppendEnvVar(envs, cw.kafkaConfigEnvVar(), cw.KafkaConfig() != nil) | |
return envs | |
} | |
// maybeAppendEnvVar appends an EnvVar only if the condition boolean is true. | |
func maybeAppendEnvVar(envs []corev1.EnvVar, env corev1.EnvVar, cond bool) []corev1.EnvVar { | |
if !cond { | |
return envs | |
} | |
return append(envs, env) | |
} | |
if cw.KafkaConfig() != nil { | |
envs = append(envs, cw.kafkaConfigEnvVar()) | |
} | |
return envs | |
} |
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.
Frankly, it is a copy paste of a private function in eventing. Let me see if I can make that one in eventing public.
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.
Hmm, I will check that after this PR as a follow up.
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.
@aliok ignore pull-knative-sandbox-eventing-kafka-integration-test-mt-source. It has been enabled yesterday. I'll take a look at it ASAP. |
WithDefaults(). | ||
WithAuth(kafkaAuthCfg). | ||
WithClientId(clientID). | ||
FromYaml(kafkaConfig.SaramaSettingsYamlString). |
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.
Can we just update the MakeAdminClient
to have that option passed in? or not, if nil
?
Feels a bit weird to duplicate that code
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.
Not duped anymore. Moved into a common place
pkg/source/client/client.go
Outdated
@@ -111,6 +126,7 @@ func MakeAdminClient(ctx context.Context, clientID string, kafkaAuthCfg *client. | |||
WithDefaults(). | |||
WithAuth(kafkaAuthCfg). | |||
WithClientId(clientID). | |||
FromYaml(kafkaConfig.SaramaSettingsYamlString). |
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.
Isn't that the public MakeAdminClient
I talked about above ?
To prevent informer injection happening whenever another package uses this code
4aa0bc8
to
276bca6
Compare
…reated before for config Kafka source client
The following is the coverage report on the affected files.
|
@aliok: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@@ -592,7 +591,7 @@ func (r *Reconciler) updateKafkaConfig(ctx context.Context, configMap *corev1.Co | |||
r.kafkaConfig = kafkaConfig | |||
r.kafkaConfigError = err | |||
ac, err := kafka.NewAdminClient(ctx, func() (sarama.ClusterAdmin, error) { | |||
return source.MakeAdminClient(ctx, controllerAgentName, r.kafkaAuthConfig, kafkaConfig) | |||
return client.MakeAdminClient(ctx, controllerAgentName, r.kafkaAuthConfig, kafkaConfig.SaramaSettingsYamlString, kafkaConfig.Brokers) |
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
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aliok, matzew The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@slinkydeveloper /unhold? |
/unhold |
To reduce the noise for other approvers on automatically requested reviews
Fixes #298
Proposed Changes
Sarama
field inconfig-kafka
in KafkaSource as defaultsRelease Note
Docs