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.

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

WIP: ReactorKafka auto-configuration (approach 1) #31258

Open
wants to merge 2 commits into
base: 2.7.x
Choose a base branch
from

Conversation

onobc
Copy link
Contributor

@onobc onobc commented Jun 7, 2022

This is a WIP proposal for ReactorKafka auto-configuration. It is a modification of the original proposal in that it leverages the already existing producer/consumer from KafkaProperties as well as supports property hierarchy of KafkaProperties.

There are still tests that need to be fleshed out but I wanted to get something for us to look at before proceeding.

Commit 1

Has nothing to do w/ ReactorKafka and is strictly a refactor to pull up common properties for re-use. This is leveraged by the 2nd commit but it useful on its own.

Commit 2

Add the ReactorKafka auto-config.

NOTE: I know the "ship has sailed" on this going into 2.7x but I left it here (FOR NOW) as ReactorKafka has not yet made it into the latest Reactor BOM for SB3 (seems to have issues w/ Java17 from 1st glance).

NOTE: This approach inserts the RK props in the KP hierarchy - the same approach used by Admin, Producer, Consumer, and Streams clients.

(+) Behaves like the other Kafka clients do and should be easy for Spring Kafka users to grok
(+) Allows setting producer/consumer at higher level (or not) and sharing (or not) w/ non-reactor clients
(-) The extra layer of overrides (hierarchy) complicates things a bit

@garyrussell I know you advocated for a separate RKP and that would be easy to implement w/ this refactored base properties in commit 1. I do think this approach feels very much like the other clients config props though.

cc: @wilkinsona

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 7, 2022
return senderOptions;
}

private <T, R> R toInstanceSafe(PropertyMapper.Source<T> source, Function<T, R> target, R defaultOptions) {
Copy link
Contributor Author

@onobc onobc Jun 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[UNRELATED] I am going to submit a separate PR to add an API to Source.toInstance that accepts a default instance when the value was filtered.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security: ...
properties: ...

reactor:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[TEMPORARY]
This file only exists to illustrate the "full" yaml available.

Map<String, Object> properties = buildCommonProperties();
properties.putAll(this.consumer.buildProperties());
public Map<String, Object> buildAdminProperties() {
// spring.kafka.<common-props>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ will probably yank these prefix/path comments out later - just easily shows what the mapping is for this proposal review only. It may be helpful to move them up to the method javadoc though in some capacity.

* @author Chris Bono
* @since 2.7.0
*/
class KafkaPropertiesBaseWithBootstrapServersAndProducerConsumer extends KafkaPropertiesBaseWithBootstrapServers {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ This hierarchy is a bit forced and brittle by nature of the config properties method / naming convention. Using inheritance makes the get/set available. Otherwise I would have explored composition.

@snicoll
Copy link
Member

snicoll commented Jul 18, 2022

@onobc thank you for the PR but why can't we continue collaborating on #30567?

@onobc
Copy link
Contributor Author

onobc commented Jul 18, 2022

@onobc thank you for the PR but why can't we continue collaborating on #30567?

Hi @snicoll -
The direction was unclear and there were enough questions that we wanted to stop the existing proposal to allow some exploration of approaches. The propsals are fairly divergent at this point so I think it makes sense to focus on this more recent one. I thought that we closed the other PR but I see it is open still which is a bit confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants