Skip to content

Conversation

@PlugaruT
Copy link

Changes

  • Full implementation of Kafka bindings

One line description for the changelog

  • Tests pass
  • Appropriate changes to README are included in PR

@PlugaruT
Copy link
Author

@xSAVIKx here's another one.

Another topic that I'd like to discuss is what would be the approach next? I'd like to remove entirely v1 package from v2 branch and start working towards a possible release? Since I think both v1 and v2 would be with the same feature set.

Also, what's your take on the protocol implementation? I took a look at go sdk for example, and they implemented the full Kafka protocol and integrated it with confluent-kafka-go lib for example, I assume same can be said about MQTT and other protocols. Do we plan to add support for this? Or, we want to keep the package light and allow users to define the protocol how they see fit?

@PlugaruT
Copy link
Author

PlugaruT commented Dec 9, 2025

@xSAVIKx ping, in case you somehow missed the notification. thanks

@xSAVIKx
Copy link
Member

xSAVIKx commented Dec 9, 2025

Hey @PlugaruT , thx for the ping. I did miss it.

I will try to get back with an answer and a review later today. Thank you 🙏

Copy link
Member

@xSAVIKx xSAVIKx left a comment

Choose a reason for hiding this comment

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

@PlugaruT LGTM 👍

Let me know if you want to handle those suggestions. But it can be done separately as well.

Comment on lines +125 to +131
def from_binary(
message: KafkaMessage,
event_format: Format,
event_factory: Callable[
[dict[str, Any], dict[str, Any] | str | bytes | None], BaseCloudEvent
],
) -> BaseCloudEvent:
Copy link
Member

Choose a reason for hiding this comment

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

this is a perfectly correct implementation, I was just thinking that 99% of users will do what you added in the example:

from_binary(message, JSONFormat(), CloudEvent)

It probably is a good idea to have this shortcut for them as well. Maybe with smth like:

cloud_event_binary_from(message)

WDYT?

Comment on lines +228 to +234
def from_structured(
message: KafkaMessage,
event_format: Format,
event_factory: Callable[
[dict[str, Any], dict[str, Any] | str | bytes | None], BaseCloudEvent
],
) -> BaseCloudEvent:
Copy link
Member

Choose a reason for hiding this comment

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

and similarly here we can provide a shortcut like:

cloud_event_structured_from(message)

@xSAVIKx
Copy link
Member

xSAVIKx commented Dec 11, 2025

Another topic that I'd like to discuss is what would be the approach next? I'd like to remove entirely v1 package from v2 branch and start working towards a possible release? Since I think both v1 and v2 would be with the same feature set.

IMO it is a good idea to release a first v2 version with v1 compatibility package. And then remove the compatibility package in a follow-up release.

Also, what's your take on the protocol implementation? I took a look at go sdk for example, and they implemented the full Kafka protocol and integrated it with confluent-kafka-go lib for example, I assume same can be said about MQTT and other protocols. Do we plan to add support for this? Or, we want to keep the package light and allow users to define the protocol how they see fit?

the main package should be kept lightweight - that's for sure. we can implement the full support for a desired library/libraries as optional deps or separate packages IMO.

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