Skip to content
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

Support Boot 1.5 configuration properties for Kafka #73

Closed
mbogoevici opened this issue Dec 14, 2016 · 8 comments
Closed

Support Boot 1.5 configuration properties for Kafka #73

mbogoevici opened this issue Dec 14, 2016 · 8 comments
Assignees
@ilayaperumalg
Milestone
1.2.0.RC1

Comments

@mbogoevici
Copy link
Contributor

mbogoevici commented Dec 14, 2016 •
edited by sabbyanandan

Spring Boot 1.5 is introducing Kafka support, so we should be able to support the Kafka autoconfiguration the same way as we do with Rabbit.
@mbogoevici mbogoevici added this to the 1.2.0.M1 milestone Dec 14, 2016
@ilayaperumalg ilayaperumalg self-assigned this Dec 14, 2016
@mbogoevici
Copy link
Contributor Author

This should maintain backwards compatibility with 1.4 - so if Boot 1.4 is on the classpath instead of Boot 1.5, we should fall back to the current configuration options. The typical use case is a Spring Cloud user using Dalston with Spring Boot 1.4.

@sabbyanandan sabbyanandan modified the milestones: FUTURE, 1.2.0.M1 Jan 5, 2017
@sabbyanandan sabbyanandan changed the title Support Boot 1.5 configuration properties for Kafka Support Boot 1.5 configuration properties for Kafka Jan 5, 2017
@sabbyanandan sabbyanandan modified the milestones: 1.2.0.M2, FUTURE Jan 31, 2017
@ilayaperumalg
Copy link
Contributor

Based on some preliminary work on this, I realised that we need to have common default values for SCSt Kafka Producer/Consumer properties and KafkaProperties from auto configuration. For instance, SCSt KafkaProducerProperties has keySerializer default value ByteArraySerializerwhereas the spring-boot keySerializer default value StringSerializer. It is difficult to know if the values comes out of default or user sets it explcitly.

May be we don't set any default values in the AutoConfiguration KafkaProperties at all?

@mbogoevici
Copy link
Contributor Author

We may override Spring Boot defaults and document how that happens in order to preserve the current behaviour of Spring Cloud Stream. What is important is for users to be able at least, to use the same property names for configuration.

@ilayaperumalg
Copy link
Contributor

I think overriding Spring Boot defaults may not be a better option as we may not know whether the Spring Boot KafkaProperties carries the defaults or the user specified ones.

Let's consider the following scenario:

We set the kafka producer key/value serialiser to ByteArraySerializer by default at SCSt while KafkaProperties form Spring Boot has the default value StringSerializer. If we want to override the KafkaProperties, we wouldn't be sure if StingSerializer is coming the default value or is it from the value set by the user.

@mbogoevici
Copy link
Contributor Author

mbogoevici commented Feb 8, 2017 •
edited

We definitely cannot use StringSerializer as the default, so that's out of question.

@ilayaperumalg
Copy link
Contributor

Yeah, that's the reason I am thinking it would be better to completely remove the defaults in Spring Boot KafkaProperties.

@artembilan
Copy link
Contributor

You can use EnvironmentPostProcessor to override spring.kafka.producer.keySerializer if it doesn't exist in the Environment yet.

@ilayaperumalg
Copy link
Contributor

@artembilan that's a good idea. I will check that out. Thank you!

ilayaperumalg added a commit to ilayaperumalg/spring-cloud-stream-binder-kafka that referenced this issue Feb 13, 2017
@ilayaperumalg
Support Spring Boot KafkaProperties
3ab636e 
 - If KafkaProperties for the KafkaAutoConfiguration is set, then use those properties for the KafkaMessageChannelBinder
 - For the KafkaProperties that have explicit default, override with the KafkaMessageChannelBinder defaults when the properties are not set by any of the property sources
 - Support the existing Kafka Producer/Consumer properties if they are set
 - Add tests

Resolves spring-cloud#73
@ilayaperumalg ilayaperumalg mentioned this issue Feb 13, 2017
Closed
ilayaperumalg added a commit to ilayaperumalg/spring-cloud-stream-binder-kafka that referenced this issue Feb 15, 2017
@ilayaperumalg
Support Spring Boot KafkaProperties
7a822ce 
 - If KafkaProperties for the KafkaAutoConfiguration is set, then use those properties for the KafkaMessageChannelBinder
 - For the KafkaProperties that have explicit default, override with the KafkaMessageChannelBinder defaults when the properties are not set by any of the property sources
 - Support the existing Kafka Producer/Consumer properties if they are set
 - Add tests

Resolves spring-cloud#73
@sobychacko sobychacko removed the in pr label Feb 16, 2017
@ilayaperumalg ilayaperumalg reopened this Feb 21, 2017
ilayaperumalg added a commit to ilayaperumalg/spring-cloud-stream-binder-kafka that referenced this issue Feb 21, 2017
@ilayaperumalg
Support KafkaProperties from Spring Boot autoconfiguration
212cf37 
 - If KafkaProperties is available from KafkaAutoConfiguration, retrieve the kafka properties and set as `KafkaBinderConfigurationProperties`' configuration property.
This way, these properties are available at the top level and per-binding properties could still override when setting producer/consumer properties during binding operation
 - Add tests to verify the scenarios

Resolves spring-cloud#73
@ilayaperumalg ilayaperumalg mentioned this issue Feb 21, 2017
Closed
@mbogoevici mbogoevici modified the milestones: 1.2.0.RC1, 1.2.0.M2 Feb 21, 2017
@sabbyanandan sabbyanandan removed the in pr label Mar 6, 2017
@sobychacko sobychacko removed the in pr label Mar 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ilayaperumalg ilayaperumalg

Labels
None yet
Projects
None yet
Milestone
1.2.0.RC1
Development

No branches or pull requests
5 participants
@ilayaperumalg @mbogoevici @sobychacko @artembilan @sabbyanandan