Conversation
torbsto
left a comment
There was a problem hiding this comment.
I would slightly prefer a new parameter in streams that lets you set the consumer group instead of the flag, and I really don't see why it would break anything.
| streams: | ||
| # brokers: "test:9092" | ||
| # schemaRegistryUrl: "url:1234" | ||
| passApplicationId: true |
There was a problem hiding this comment.
I think the default should be false if your goal is that it is non-breaking
There was a problem hiding this comment.
New helm chart should have it as default but old apps can still use new chart. We did a similar thing for optimizeLeaveGroupBehavior
| return kafkaConfig; | ||
| } | ||
|
|
||
| private String getApplicationId() { |
There was a problem hiding this comment.
I guess it would clutter the API even more and lead to confusion together with getUniqueAppId(), but I feel like we will need to call it elsewhere
There was a problem hiding this comment.
Hm, this method does the validation whenever Kafka properties are created. So getUniqueAppId() should still return the correct result unless a user explicitly overrides the method and it returns null. I don't think this will ever happen
| return kafkaConfig; | ||
| } | ||
|
|
||
| private String getApplicationId() { |
There was a problem hiding this comment.
And I think we need a test for this method
There was a problem hiding this comment.
Will add tests, just wanted to have your thoughts first
|
@raminqaf @philipp94831 Like I said above, I'd prefer using a separate argument for the app id and not reusing the autoscaling one. See feature/application-id-param...feature/helm-app-id for how I imagine it could work |
I feel like having two parameters is more confusing. Then we should document it well and probably deprecat the old way of configuring it |
|
I would be fine with deprecating it, but I'm not sure if there might be a situation in which someone wants to use KEDA but not set the consumer group via Helm. But I agree that we should also document it properly in the values.yaml and README.md |
There is also this annotation we should not forget about https://github.com/bakdata/streams-bootstrap/blob/master/charts/streams-app/templates/deployment.yaml#L20 |
|
Closing in favor of #243 |
No description provided.