Add --unique-app-id parameter #197
Conversation
streams-bootstrap/src/main/java/com/bakdata/kafka/KafkaStreamsApplication.java
Outdated
Show resolved
Hide resolved
streams-bootstrap/src/main/java/com/bakdata/kafka/KafkaStreamsApplication.java
Show resolved
Hide resolved
streams-bootstrap/src/main/java/com/bakdata/kafka/KafkaStreamsApplication.java
Outdated
Show resolved
Hide resolved
philipp94831
changed the title
| @@ -15,6 +15,7 @@ configurationEnvPrefix: "APP" | |||
| 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
|
|
||
| this.configureDefaultSerde(kafkaConfig); | ||
| kafkaConfig.setProperty(StreamsConfig.BOOTSTRAP_SERVERS_CONFIG, this.getBrokers()); | ||
| 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
|
|
||
| this.configureDefaultSerde(kafkaConfig); | ||
| kafkaConfig.setProperty(StreamsConfig.BOOTSTRAP_SERVERS_CONFIG, this.getBrokers()); | ||
| 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.