Remove SslBundles from KafkaProperties as it is no longer used
This PR removes the deprecated buildConsumerProperties(SslBundles) method from the KafkaProperties.
The removed test has been restored and updated to reflect the new logic. All tests and builds pass locally.
assertThatExceptionOfType(MutuallyExclusiveConfigurationPropertiesException.class).isThrownBy(properties::buildConsumerProperties);
I’d like to ask for guidance regarding the proper direction for this change.
Since buildConsumerProperties(SslBundles) has been removed, I'm deciding between two implementation options for buildConsumerProperties().
Option 1 – Minimal change (currently applied in PR):
public Map<String, Object> buildConsumerProperties() {
Map<String, Object> properties = buildCommonProperties(null);
properties.putAll(this.consumer.buildProperties(null));
return properties;
}
This keeps the current method signature of buildProperties(SslBundles) and simply passes null, minimizing code changes.
Option 2 – Cleanup approach:
public Map<String, Object> buildConsumerProperties() {
Map<String, Object> properties = buildCommonProperties(null);
properties.putAll(this.consumer.buildProperties());
return properties;
}
public static class Consumer {
private final Ssl ssl = new Ssl();
...
public Map<String, Object> buildProperties() {
Properties properties = new Properties();
PropertyMapper map = PropertyMapper.get().alwaysApplyingWhenNonNull();
map.from(this::getAutoCommitInterval)
.asInt(Duration::toMillis)
...
.to(properties.in(ConsumerConfig.MAX_POLL_INTERVAL_MS_CONFIG));
return properties.with(this.ssl, this.security, this.properties, null);
}
}
This removes the now-unnecessary parameter entirely and results in a cleaner method.
Functionally, both options appear to be equivalent.
I'd love to hear your thoughts. Thank you!
Thanks for noticing this, @Torres-09. I think we should go for the second option. SslBundles appears in the signature of several methods in KafkaProperties but is never actually used. It looks like all references to SslBundles can be removed from KafkaProperties. This will require some small changes to its tests and a couple of configuration classes as well.
@wilkinsona
Thank you for the feedback!
I’ll proceed to remove all remaining references to SslBundles from KafkaProperties, as well as update the related classes and tests accordingly. I’ll revise the PR to reflect these changes.
@wilkinsona 👋 I accidentally marked this conversation as resolved and forgot to leave a comment — sorry about that.
Thanks for the review. I've restored the four KafkaPropertiesTests and updated them using .isThrownBy(properties::buildConsumerProperties).
All tests pass locally. Let me know if anything else needs to be adjusted!
Does the same problem not also apply for buildProducerProperties.
Should buildProducerProperties(SslBundles sslBundles) not be removed?
@wesboe the changes proposed here already do that
Good stuff @Torres-09, thank you!
Closes by https://github.com/spring-projects/spring-boot/commit/c86415346d1241846ab8d0120e9d3bae50bc9314