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

Replace Vert.x Kafka client with native Java Kafka client in the sink endpoint #734

Merged
merged 7 commits into from
Dec 22, 2022

Conversation

ppatierno
Copy link
Member

@ppatierno ppatierno commented Dec 20, 2022

This PR is about replacing the usage of the Vert.x Kafka client on the sink endpoint (consumer side of the bridge) with the native Java Kafka client (provided by Apache Kafka upstream project).

It provides the following things:

  • using CompletableFuture(s) based pattern to run consumer requests asynchronously (as today it's done by Vert.x)
  • removing the Vert.x instance from the sink/consumer side
  • replacing Vert.x Kafka related classes, i.e. KafkaConsumer, KafkaConsumerRecord, and more with the corresponding native ones from the Java Kafka client

As additional note, I decided to start the async operations (via CompletableFuture.runAsync and similar) in the higher level class HttpSinkBridgeEndpoint because, as already raised by Tom in the producer side, we could end at some point to remove the SinkBridgeEndpoint class, having just one. This is also the same behaviour as the producer/source endpoint.

Anyway, there are still some Vert.x structures in place like the ones handling JSON, Buffer(s) and Handler(s).

@ppatierno ppatierno requested review from tombentley, scholzj and a team December 20, 2022 17:09
@ppatierno ppatierno marked this pull request as draft December 20, 2022 17:27
Signed-off-by: Paolo Patierno <[email protected]>
@ppatierno ppatierno added this to the 0.24.0 milestone Dec 20, 2022
@ppatierno ppatierno marked this pull request as ready for review December 21, 2022 08:27
Signed-off-by: Paolo Patierno <[email protected]>
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

LGTM. But I only run through it quickly ... should be probably also reviewed by Tom ;-)

@ppatierno
Copy link
Member Author

Thanks @scholzj ... yeah Tom already had a first pass. @tombentley I addressed your comments, any more feedback on this one?

Copy link
Member

@tombentley tombentley left a comment

Choose a reason for hiding this comment

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

Left a few nits, but otherwise LGTM.

Signed-off-by: Paolo Patierno <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants