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: add kafka support #271 #340

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

Grongrilla
Copy link
Contributor

No description provided.

@Grongrilla
Copy link
Contributor Author

Grongrilla commented Sep 13, 2024

This is a WIP PR

tested

Locally running kafka (docker) recieves the same CloudEvents messages as the NATS events backend on the configured topic. The configuration of the initial brokers with a single host:port entry works.

open or tbd

  • Final decision to use FutureProvider or BasicProvider; remove unused provider to fix clippy errors
  • No authentication supported
  • No fine grained control over producer configuration
  • README.MD not yet upated with new configuration environment variables
  • Configuration of multiple initial brokers not tested
  • No SSL support
  • Decide if current location for vendored code is fine, or move to better location (maybe kafka module below service module
  • ...?

@Grongrilla Grongrilla marked this pull request as draft September 13, 2024 18:52
@@ -1,8 +1,8 @@
[workspace]
members = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we get rid of the formatting changes here? It makes it a bit more difficult to discern actual changes from whitespace changes & we'll likely end up with a bit of ping pong with formatting between PRs. We could consider adopting https://github.com/tamasfe/taplo to format tomls.

@Grongrilla
Copy link
Contributor Author

@twuebi are you Ok with the way I introduced the vendored code or would you prefer something different?

@twuebi
Copy link
Contributor

twuebi commented Sep 16, 2024

@twuebi are you Ok with the way I introduced the vendored code or would you prefer something different?

Maybe we promote service::event_publishers to a directory module and then have submodules for nats & kafka where kafka has a submodule vendored? IMO the vendored stuff is a detail of the kafka module and we could just stuff it in there, what do you think?

@Grongrilla
Copy link
Contributor Author

@twuebi are you Ok with the way I introduced the vendored code or would you prefer something different?

Maybe we promote service::event_publishers to a directory module and then have submodules for nats & kafka where kafka has a submodule vendored? IMO the vendored stuff is a detail of the kafka module and we could just stuff it in there, what do you think?

The modules would be very small, especially after vendored is gone (which will happen sometimes, I hope). I still like this idea better than putting the vendored code from Cloudevents SDK in its' own module. I will restructure the code as you suggested :)

@Grongrilla
Copy link
Contributor Author

Regarding the discussion about brokers and topics, I found this helpfull: Kafka broker and rdkafka Configuration.

So to my understanding, a Kafka deployment can have multiple brokers, but they all know the same topics. The configuration parameter of rdkafka seems to expect the list of brokers to belong to the same deployment - it is not explicetly stated as such, but there is no way to configure a topic->broker mapping in rdkafka. The topic is part of the message, not the producer instance.

It could be possible that a TIP users wants to publish to multiple topics, which might life on multiple kafka deployments. The same is probably true for NATS.... but this sounds like an exception to me that we probably do not need to support out of the box.

But on taht topic, another question: do we want to test kafka support with "just" kafka, or also with redpanda?

@twuebi
Copy link
Contributor

twuebi commented Sep 23, 2024

But on taht topic, another question: do we want to test kafka support with "just" kafka, or also with redpanda?

is one much more work than the other? In general, testing against more implementations is nice, on the other hand, I'd defer it to a follow-up if it makes the scope of this PR bigger.

- No authentication supported
- No fine grained control over producer configuration
- README.MD not yet upated with new configuration environment variables
- ...
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.

2 participants