Add Kafka AutoConfiguration #6961
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
| @@ -0,0 +1 @@ | |||
| Test file for Kafka. | |||
There was a problem hiding this comment.
Test file
But it is somehow in the main...
garyrussell
force-pushed
the
kafkaAutoConfig
branch
from
c3927ac to
93494a3
Compare
|
Hi @garyrussell , @artembilan Just spotted this pull request. What would you recommend regarding the metrics which I've talked about before? |
|
@stepio , Glad to hear from you back again! As you see this Kafka auto-configuration hasn't been merged yet. Thank you for the contribution as usual! |
snicoll
added
type: enhancement
There was a problem hiding this comment.
Hey @garyrussell thanks for the PR! I've added some comments and I am happy to process them myself but as I am quite new to Kafka I want to make sure I didn't overlook something first.
| */ | ||
| public void setKafkaProperties(KafkaProperties properties) { | ||
| this.properties = properties; | ||
| } |
There was a problem hiding this comment.
Is there a reason why this is not set via constructor injection? If you make the link with the JMS part we had a bunch of components that might be responsible of tuning the factory and we didn't want to offer a way to users to modify the configurer once it has been created. That's why they are package protected (this one is public).
There was a problem hiding this comment.
I based this on the amqp auto config but I missed that it has package protection for these methods.
| @ConditionalOnMissingBean(KafkaTemplate.class) | ||
| public KafkaTemplate<?, ?> kafkaTemplate(ProducerFactory<Object, Object> kafkaProducerFactory, | ||
| ProducerListener<Object, Object> kafkaProducerListener) { | ||
| KafkaTemplate<Object, Object> kafkaTemplate = new KafkaTemplate<Object, Object>(kafkaProducerFactory); |
There was a problem hiding this comment.
Is having more than one ProducerFactory and/or ProducerListener a common thing? If that's the case, can we please switch to a pattern where we use @ConditionalOnSingleCandidate? you'll need a custom condition if you want to apply that to the two instances. If that does not make sense to have more than one instance of those, the code is fine as is.
There was a problem hiding this comment.
No, it wouldn't be common, unless you are talking to two or more distinct kafka clusters (which would need all the other configs as well). In fact, kafka recommends a single producer instance per JVM (which is what the default factory serves up).
| * A comma-delimited list of host:port pairs to use for establishing the initial | ||
| * connection to the Kafka cluster. | ||
| */ | ||
| private List<String> bootstrapServers = Collections.singletonList("localhost:9092"); |
There was a problem hiding this comment.
you need to wrap that in a new ArrayList otherwise access by key won't work (the initial collection is immutable).
There was a problem hiding this comment.
I am not sure what you mean by "access by key" but I can add the wrapper.
| /** | ||
| * The password of the private key in the key store file. | ||
| */ | ||
| private String sslKeyPassword; |
There was a problem hiding this comment.
Can we have a ssl namespace for these (see RabbitProperties for instance)
| private String clientId; | ||
|
|
||
| /** | ||
| * The password of the private key in the key store file. |
There was a problem hiding this comment.
Meta-data description never starts with The, A, etc. Could you please align the documentation of those keys so that they match what we already have?
| /** | ||
| * The password of the private key in the key store file. | ||
| */ | ||
| private String sslKeyPassword; |
| /** | ||
| * The store password for the trust store file. | ||
| */ | ||
| private String sslTruststorePassword; |
| "spring.kafka.listener.concurrency=3", | ||
| "spring.kafka.listener.poll-timeout=2000" | ||
| }) | ||
| public class KafkaPropertiesTests { |
There was a problem hiding this comment.
Such Properties object are rather tested as simple unit test, using EnvironmentContextTestUtils to bind the relevant keys.
| # Apache Kafka designates properties with an importance: HIGH, MEDIUM and LOW. | ||
| # Spring Boot auto configuration supports all HIGH importance properties, some selected MEDIUM and LOW, | ||
| # and any that do not have a default value. | ||
| # To set additional properties, override the consumer and/or producer factory beans; see <<kafka-extra-props>>. |
There was a problem hiding this comment.
I would rather see that text in the documentation itself. We really intend this appendix to only list the keys and their documentation (we have the intend to actually auto-generate it).
| @@ -4499,7 +4499,77 @@ Here's an example of a customizer that configures the use of a proxy for all hos | |||
| include::{code-examples}/web/client/RestTemplateProxyCustomizationExample.java[tag=customizer] | |||
| ---- | |||
|
|
|||
| [[boot-features-kafka]] | |||
| == Apache Kafka | |||
There was a problem hiding this comment.
Any reason why it's not located under the messaging section?
Change Common Props to Hyphenated (asciidoc) Only Support HIGH and a Few Other Props
garyrussell
force-pushed
the
kafkaAutoConfig
branch
from
93494a3 to
dc114aa
Compare
* pr/6961: Polish "Apache Kafka support" contribution Add Apache Kafka support
Replaces #5516
Change Common Props to Hyphenated (asciidoc)
Only Support HIGH and a Few Other Props