-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Expose GSSAPI Service name configuration of Kafka Scaler #5483
Expose GSSAPI Service name configuration of Kafka Scaler #5483
Conversation
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 for the contribution!
We need to parse the value from triggerMetadata because currently serviceName
is always ""
. A unit test to cover this would be nice too.
Last but not least, we have to open another PR to docs repo adding the info: https://github.com/kedacore/keda-docs/blob/main/content/docs/2.14/scalers/apache-kafka.md
Thank you for the hint, I have now updated the code to read from the base configuration. I believe it should be part of the auth params and not trigger Metadata, since it is a Kerberos configuration? Please correct me if my understanding is incorrect. As per your request, I have raised a separate pull request for the documentation . |
/run-e2e kafka |
015fd23
to
6834379
Compare
Sorry, Done. |
/run-e2e kafka |
@dttung2905 @zroubalik , WDYT? |
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.
Hi @ArunYogesh , thanks for the PR. Overall I think its a good change. I have a few nits:
- Could you help to make changes to https://github.com/kedacore/keda/blob/main/pkg/scalers/apache_kafka_scaler.go too ? We currently support 2 kafka version (kafka_scaler using sarama library and apache_kafka_scaler using kafka-go)
- Could you help to add some tests for it too, probably in this ?
keda/pkg/scalers/kafka_scaler_test.go
Line 331 in cdbcb9f
func getBrokerTestBase(t *testing.T, meta kafkaMetadata, testData parseKafkaMetadataTestData, err error) { - We also need to update the doc for it as well.
19a07cf
to
b1da4a8
Compare
I guess that we can continue with the current implementation without adding the support to the experimental, as after the last community call we plan to deprecate and remove the experimental one. WDYT @zroubalik @tomkerkhove @dttung2905 ? For me, once the tests' requirement is done we can merge this if you agree |
b1da4a8
to
dca7209
Compare
Signed-off-by: Arun Yogesh <6723526+ArunYogesh@users.noreply.github.com>
dca7209
to
c9b0502
Compare
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!
/run-e2e kafka |
Thanks for the contribution! |
Thank you all! |
Signed-off-by: Arun Yogesh <6723526+ArunYogesh@users.noreply.github.com>
The current GSSAPI configuration for Kafka scaler has a hard coded service name. The service name can be different in some Kafka configurations, hence exposing it to be configurable.
Checklist
Fixes #5474