Add auto-configuation for kafka-streams #14021

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.

Sign up for GitHub

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

Jump to bottom

Closed

garyrussell wants to merge 3 commits into spring-projects:master from garyrussell:kafkaStreams

Contributor

garyrussell commented Aug 8, 2018

If kafka-streams is on the classpath, auto configure Kafka Streams
Properties, set @EnableKafkaStreams and disable the factory
bean's autoStartup property.

spring-projects-issues added the status: waiting-for-triage label

snicoll reviewed

View reviewed changes

Member

snicoll left a comment

Thanks for the PR @garyrussell - We should probably add dependency management for Kafka streams as part of this change.

I've added a few comments, let me know what you think.

...igure/src/main/java/org/springframework/boot/autoconfigure/kafka/KafkaAutoConfiguration.java Outdated

    
              	public static class KafkaStreamsConfiguration {

              		@Bean(KafkaStreamsDefaultConfiguration.DEFAULT_STREAMS_CONFIG_BEAN_NAME)

              		public Properties defaultKafkaStreamsConfig(KafkaProperties properties) {

Member

snicoll Aug 9, 2018

I am not keen to expose a @Bean of type Properties, perhaps we could have a dedicated object for this or does the underlying configuration actually require this?

Contributor Author

garyrussell Aug 9, 2018

Kafka deprecated the use of an immutable StreamsConfig in favor of a Properties object to configure the StreamsBuilder. I can add a wrapper class in spring-kafka if you prefer.

https://jira.apache.org/jira/browse/KAFKA-6386

Member

snicoll Aug 9, 2018 •

edited

Loading

Yes please, Properties is way to generic to be exposed as a bean. Ideally this should be done in spring-kafka itself as I imagine this would be larger in scope.

...igure/src/main/java/org/springframework/boot/autoconfigure/kafka/KafkaAutoConfiguration.java Outdated

    
              		}

              		@Bean

              		public Object kafkaStreamsFactoryBeanConfigurer(

Member

snicoll Aug 9, 2018

Object ?

Contributor Author

garyrussell Aug 9, 2018

I used a NullBean (return null) to avoid creating an explicit ...Configurer class just for this one property; I can do that if you prefer.

Member

snicoll Aug 9, 2018 •

edited

Loading

I am not sure I understand what that means but I'd rather have something that backs off with a property type if we should do something.

...utoconfigure/src/main/java/org/springframework/boot/autoconfigure/kafka/KafkaProperties.java Outdated

    
              import org.apache.kafka.common.config.SslConfigs;

              import org.apache.kafka.common.serialization.StringDeserializer;

              import org.apache.kafka.common.serialization.StringSerializer;

              import org.apache.kafka.streams.StreamsConfig;

Member

snicoll Aug 9, 2018

That requires Kafka streams to be on the classpath and, as I understand, this is an optional dependency on top of Kafka

Contributor Author

garyrussell Aug 9, 2018

Oops

Member

artembilan Aug 10, 2018

More over according some previous comment this class is deprecated there.

...utoconfigure/src/main/java/org/springframework/boot/autoconfigure/kafka/KafkaProperties.java Outdated

    
              		/**

              		 * Kafka streams application.id property; default spring.application.name.

              		 */

              		private String applicationId = "${spring.application.name}";

Member

snicoll Aug 9, 2018 •

edited

Loading

Is that supposed to be SpEL? I'd prefer if we perform that detection elsewhere (in the caller with access to the Environment).

Contributor Author

garyrussell Aug 9, 2018

I used a property placeholder which gets resolved as expected in the test case; I can move it to the caller.

Member

snicoll Aug 9, 2018

We don't do property placeholder in properties. Yes please.

...utoconfigure/src/main/java/org/springframework/boot/autoconfigure/kafka/KafkaProperties.java Outdated

    
              		private String applicationId = "${spring.application.name}";

              		/**

              		 * Whether or not to auto-start the streams factory bean; default false.

Member

snicoll Aug 9, 2018

We don't add default values to descriptions.

Contributor Author

garyrussell Aug 9, 2018

Since autoStartup is usually true by default; I thought it was important to add it here. The reason it's false is I don't want to start the factory bean for an existing Boot app that uses streams but doesn't use the auto configured infrastructure (start connects to Kafka).

I did mention it in the docs, however, so I can remove it here.

Member

snicoll Aug 9, 2018

The point I was trying to make is that the metadata will already state the default is false (as inferred by the default value of the field). So your IDE will tell you it's false and having that in text is a duplicate that could potentially get out of sync.

Does that make sense?

...utoconfigure/src/main/java/org/springframework/boot/autoconfigure/kafka/KafkaProperties.java

    
              		 * Comma-delimited list of host:port pairs to use for establishing the initial

              		 * connection to the Kafka cluster.

              		 */

              		private List<String> bootstrapServers;

Member

snicoll Aug 9, 2018

I am confused. Is that streams specific? Yet this property doesn't indicate that's the case.

Contributor Author

garyrussell Aug 9, 2018

This is similar to producer/consumer - we can add a global property and/or override at the component level - I will enhance these docs in all 3 places.

...utoconfigure/src/main/java/org/springframework/boot/autoconfigure/kafka/KafkaProperties.java

    
              		private String clientId;

              		/**

              		 * The replication factor for change log topics and repartition topics created by

Member

snicoll Aug 9, 2018

Key descriptions do not start with "The", "A", etc.

Contributor Author

garyrussell Aug 9, 2018

OK; I simply copied the Kafka descriptions 😄

spring-boot-project/spring-boot-docs/src/main/asciidoc/spring-boot-features.adoc

    
              Spring for Apache Kafka provides a factory bean to create a `StreamsBuilder` object and

              manage the lifecycle of its streams; the factory bean is created when

              `@EnableKafkaStreams` is present on a `@Configuration` class.

Member

snicoll Aug 9, 2018

I can see the auto-configuration enables that so something is not consistent. Perhaps the auto-configuration should check for the presence of @EnableKafkaStreams if that makes sense? (We don't do that for Kafka so perhaps that's the wrong idea).

Contributor Author

garyrussell Aug 9, 2018

My mistake; I originally did not add @EnableKafkaStreams in the auto config because of the problem with auto-starting the factory bean.

I later added the autoStartup auto-config to prevent the FB from starting unless the user explicitly sets it, but failed to revisit the docs.

Which do you prefer - requiring a user-specified @EnableKafkaStreams or this autoStartup dance? I think I would prefer that the user adds the annotation, but that seems inconsistent with other practices.

Member

snicoll Aug 9, 2018

I don't know really. I'd like this to be consistent. On the other hand, I don't know if adding kafka-streams is enough to trigger that behaviour. Since adding spring-kafka is enough for the kafka use case, I'd be tempted to say that's the same here (you need spring-kafka and kafka-streams anyway).

Any potential clash with Spring Cloud Stream and the kafka streams story there?

Contributor Author

garyrussell Aug 9, 2018

I'll stick with the presence of the jar for consistency (and fix the configurer).

I will reach out to the SCSt team to make them aware of this.

snicoll added the status: waiting-for-feedback label

Contributor Author

garyrussell commented Aug 9, 2018

@snicoll Thanks for reviewing;

kafka-streams is already in the pom: https://github.com/spring-projects/spring-boot/blob/master/spring-boot-project/spring-boot-dependencies/pom.xml#L1489

Is something else needed?

I will address your other comments shortly; thanks.

Member

snicoll commented Aug 9, 2018

kafka-streams is already in the pom:

Wow I totally missed that. It means we can remove our hard-coded version in initializr, sweet!

Thanks for the feedback, let me know if you need help with any of this.

garyrussell added a commit to garyrussell/spring-kafka that referenced this pull request


          Add KafkaStreamsConfiguration

9abc1b0

See spring-projects/spring-boot#14021 (comment)

Avoid using a bean of type `Properties`.

garyrussell mentioned this pull request

Add KafkaStreamsConfiguration spring-projects/spring-kafka#776

Merged

Contributor Author

garyrussell commented Aug 9, 2018

I pushed a PR for spring-kafka to avoid the Properties bean; I'll address your other comments when that is merged.

Thanks.

artembilan pushed a commit to spring-projects/spring-kafka that referenced this pull request


          Add KafkaStreamsConfiguration

29df442

See spring-projects/spring-boot#14021 (comment)

Avoid using a bean of type `Properties`.

* Polishing - PR Comments

garyrussell added 3 commits

August 13, 2018 16:59


          Kafka Streams AutoConfiguration

680e27c

If `kafka-streams` is on the classpath, auto configure Kafka Streams
`Properties`, set `@EnableKafkaStreams` and disable the factory
bean's `autoStartup` property.


          Fix bootstrap.servers

009afa4

`toString()` rendered the list of servers within [...].

Remove the brackets since streams expects a comma-delimited list.


          Polishing - PR Comments

30d330d

garyrussell force-pushed the kafkaStreams branch from 775eb8f to 30d330d Compare

August 13, 2018 21:00

Contributor Author

garyrussell commented Aug 13, 2018

@snicoll I addressed your concerns.

Please note that I am on PTO this week; if this needs more polishing, please reach out to @artembilan ; I will make him a collaborator on my fork so he can push to this PR.

Thanks, Gary

snicoll pushed a commit to snicoll/spring-boot that referenced this pull request


          Add Kafka Streams auto-configuration

2df9cc5

See spring-projectsgh-14021

snicoll added type: enhancement and removed status: waiting-for-feedback status: waiting-for-triage labels

snicoll self-assigned this

snicoll pushed a commit to snicoll/spring-boot that referenced this pull request


          Add Kafka Streams auto-configuration

e942fde

See spring-projectsgh-14021

snicoll added a commit to snicoll/spring-boot that referenced this pull request


          Polish "Add Kafka Streams auto-configuration"

1d113de

Closes spring-projectsgh-14021

Member

snicoll commented Aug 22, 2018

@garyrussell @artembilan I've changed my mind looking at the current proposal, please see
1d113de for more details.

Given that application.id is mandatory, I feel that the current proposal is missing some context: let's assume you upgrade and you happen to have kafka-streams on the classpath and no application id (or spring.application.name). We auto-configure things automatically, configure a bean without application.id (invalid) and not start anything so "all is fine".

Now if you enable auto-startup, you have an exception deep down that tells you application.id is not set. The link to Spring Boot is not super obvious. I am not a huge fan of overriding the default value of auto-startup.

I've modified the initial proposal so that it requires @EnableKafkaStreams and we fail hard if the application id is not set (using a failure analyzer that tells you exactly what happened). The auto-startup flag has also been restored to its default value.

Concretely, nothing happens until @EnableKafkaStreams is set. When it's set, the configuration is validated upfront.

What do you think about this proposal? Does that make sense? Thanks!

Contributor Author

garyrussell commented Aug 22, 2018

@snicoll LGTM - that was my original approach (user opt-in with @EnableKafkaStreams).

Since I thought that was inconsistent with other auto-config (adding @Enable... automatically), I went to the auto-startup dance.

I definitely prefer this solution, though.

Thanks!

Member

snicoll commented Aug 22, 2018

Sweet, thanks for the feedback @garyrussell!

snicoll added this to the 2.1.0.M3 milestone

snicoll pushed a commit that referenced this pull request


          Add Kafka Streams auto-configuration

a7acbbd

See gh-14021

snicoll closed this in

6d4bab9

snicoll added a commit that referenced this pull request


          Merge pull request #14021 from garyrussell:kafkaStreams

f8ce714

* pr/14021:
  Polish "Add Kafka Streams auto-configuration"
  Add Kafka Streams auto-configuration

philwebb changed the title ~~Kafka Streams AutoConfiguration~~ Add auto-configuation for kafka-streams

denis554 added a commit to denis554/spring-kafka that referenced this pull request


          Add KafkaStreamsConfiguration

1f5399a

See spring-projects/spring-boot#14021 (comment)

Avoid using a bean of type `Properties`.

* Polishing - PR Comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: enhancement