feat: add support for kafka transport #455

lance · 2021-12-22T19:27:31Z

This commit extends the message package to include Kafka transport. The tests included here were almost entirely copied verbatim from message_tests.ts which tests the HTTP message binding. The primary change to these tests was the use of the Kafka binding instead of the HTTP binding. It would be nice, I think, to have generic tests that can test any new transport layer by just taking the binding as a parameter. But that's not what I've done here. :) I've mostly just copied and pasted.

There is one test that is pending as it's not passing yet, and I figured it was a bit of an edge case and could be addressed later.

      - Converts base64 encoded data to binary when deserializing structured messages

Note that only JSON encoding is supported.

Additionally, some of the type information has changed across the project to more accurately reflect the type of Message with Message<T>.

Related: #390

grant

Few requested changes. Hope the comments are helpful.

Nothing particularly blocking.

I'm not familiar with the goal of the SDKs in terms of supporting all protocols / transports. It be best to keep other transport logic to child helpers since a developer will usually only use the SDK for one protocol at a time.

package-lock.json

src/message/index.ts

src/message/kafka/headers.ts

src/message/kafka/index.ts

test/integration/emitter_factory_test.ts

test/integration/emitter_singleton_test.ts

lance · 2021-12-22T22:54:06Z

Thanks for the feedback @grant - I've made some updates. Making the generic type T default to string was quite helpful - thanks for pointing that out.

lance · 2021-12-22T23:00:24Z

I'm not familiar with the goal of the SDKs in terms of supporting all protocols / transports. It be best to keep other transport logic to child helpers since a developer will usually only use the SDK for one protocol at a time.

Not sure how I feel about this. Other SDKs do support multiple protocols, and all do so within a single repository. Distributing the bits of these implementations is, of course, language specific. So, in Java, for example, you can pull in just the Kafka bindings iiuc, but that's not the case in Rust, for example, where the implementation is all together. In any case, the different transport protocol support is all implemented in a single github repo - which is what we would need to do as well. I think the additional bytes that are added to the package by these two files are really not an issue compared to the complexity of reorganizing this repo to be a mono-repo, supporting multiple npm modules. My 2c.

grant · 2021-12-22T23:08:32Z

I'm not familiar with the goal of the SDKs in terms of supporting all protocols / transports. It be best to keep other transport logic to child helpers since a developer will usually only use the SDK for one protocol at a time.

Not sure how I feel about this. Other SDKs do support multiple protocols, and all do so within a single repository. Distributing the bits of these implementations is, of course, language specific. So, in Java, for example, you can pull in just the Kafka bindings iiuc, but that's not the case in Rust, for example, where the implementation is all together. In any case, the different transport protocol support is all implemented in a single github repo - which is what we would need to do as well. I think the additional bytes that are added to the package by these two files are really not an issue compared to the complexity of reorganizing this repo to be a mono-repo, supporting multiple npm modules. My 2c.

Yeah it's fine imo. The main thing is being wary of extra dependencies that may be unnecessary, and ensuring it's still easy to contribute to HTTP features without having to affect other protocols.

LeoVS09

Sorry for breaking into your working process, but it critical bug was my failure, and it can create complex problems for production applications

src/message/kafka/index.ts

This change modifies the protocol binding interfaces such as `Binding`, `Serializer` and the like to use the `CloudEventV1` interface instead of the implementation class `CloudEvent`. This should make extending the interfaces simpler as this work has grown out of efforts around the implementation of a second transport interface, Kafka. See: cloudevents#455 This commit also includes the addition of a generic type to the `Message` interface, defaulting to `string`. There is also some minor clean up involving what is exported from the `message/http` modules. Now, instead of exporting the entire implementation, only the `HTTP` binding implementation is exported, and it is then reexported by `message`. Also, a static `CloudEvent.cloneWith()` method has been added which the instance methods now use. Signed-off-by: Lance Ball <lball@redhat.com>

lance · 2022-01-04T22:42:24Z

@LeoVS09 I've made the appropriate changes, ptal.
@grant @lholmquist a bit has changed sine your last reviews, would you mind taking another look at this? Thanks.

This change modifies the protocol binding interfaces such as `Binding`, `Serializer` and the like to use the `CloudEventV1` interface instead of the implementation class `CloudEvent`. This should make extending the interfaces simpler as this work has grown out of efforts around the implementation of a second transport interface, Kafka. See: cloudevents#455 This commit also includes the addition of a generic type to the `Message` interface, defaulting to `string`. There is also some minor clean up involving what is exported from the `message/http` modules. Now, instead of exporting the entire implementation, only the `HTTP` binding implementation is exported, and it is then reexported by `message`. Also, a static `CloudEvent.cloneWith()` method has been added which the instance methods now use. Signed-off-by: Lance Ball <lball@redhat.com>

lance · 2022-01-05T15:34:16Z

Marking as WIP until #457 is resolved as this PR depends on many of those changes, and I think it's better to keep the PRs small and focused vs. cramming all of that in here.

* chore(refactor): protocol bindings use interfaces This change modifies the protocol binding interfaces such as `Binding`, `Serializer` and the like to use the `CloudEventV1` interface instead of the implementation class `CloudEvent`. This should make extending the interfaces simpler as this work has grown out of efforts around the implementation of a second transport interface, Kafka. See: #455 This commit also includes the addition of a generic type to the `Message` interface, defaulting to `string`. There is also some minor clean up involving what is exported from the `message/http` modules. Now, instead of exporting the entire implementation, only the `HTTP` binding implementation is exported, and it is then reexported by `message`. Also, a static `CloudEvent.cloneWith()` method has been added which the instance methods now use. Signed-off-by: Lance Ball <lball@redhat.com> * fixup: make the `cloneWith()` method is dependent on interfaces Signed-off-by: Lance Ball <lball@redhat.com> * fixup: remove unnecessary cast Signed-off-by: Lance Ball <lball@redhat.com>

This commit extends the `message` package to include Kafka transport. Additionally, some of the type information has changed across the project to more accurately reflect the type of `Message` (by including `T`). Related: cloudevents#390 Signed-off-by: Lance Ball <lball@redhat.com>

lance · 2022-01-07T20:07:06Z

I have rebased with the latest changes from main and generally cleaned this up a bit. It's ready for a final review @grant and/or @lholmquist. Thanks!

lance requested a review from a team December 22, 2021 19:27

lance added module/lib Related to the main source code type/enhancement New feature or request labels Dec 22, 2021

lance force-pushed the lance/390-add-kafka branch from cf9852e to 803445f Compare December 22, 2021 19:29

grant suggested changes Dec 22, 2021

View reviewed changes

LeoVS09 suggested changes Dec 23, 2021

View reviewed changes

src/message/kafka/index.ts Outdated Show resolved Hide resolved

lance mentioned this pull request Jan 4, 2022

chore(refactor): prefer interfaces over concrete classes #457

Merged

lance force-pushed the lance/390-add-kafka branch from 86306ed to aeda5a3 Compare January 4, 2022 22:44

lance force-pushed the lance/390-add-kafka branch from aeda5a3 to 862b4b2 Compare January 4, 2022 22:47

lance marked this pull request as draft January 5, 2022 15:34

lance force-pushed the lance/390-add-kafka branch from 862b4b2 to fb6ee83 Compare January 7, 2022 19:40

lance marked this pull request as ready for review January 7, 2022 20:06

grant approved these changes Jan 7, 2022

View reviewed changes

lance merged commit 5d1f744 into cloudevents:main Jan 7, 2022

lance deleted the lance/390-add-kafka branch January 7, 2022 21:14

lholmquist mentioned this pull request Jan 10, 2022

plans for amqp/nats/stan/etc (similar to sdk-go)? #390

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for kafka transport #455

feat: add support for kafka transport #455

lance commented Dec 22, 2021 •

edited

Loading

grant left a comment •

edited

Loading

lance commented Dec 22, 2021

lance commented Dec 22, 2021

grant commented Dec 22, 2021

LeoVS09 left a comment

lance commented Jan 4, 2022

lance commented Jan 5, 2022

lance commented Jan 7, 2022

feat: add support for kafka transport #455

feat: add support for kafka transport #455

Conversation

lance commented Dec 22, 2021 • edited Loading

grant left a comment • edited Loading

Choose a reason for hiding this comment

lance commented Dec 22, 2021

lance commented Dec 22, 2021

grant commented Dec 22, 2021

LeoVS09 left a comment

Choose a reason for hiding this comment

lance commented Jan 4, 2022

lance commented Jan 5, 2022

lance commented Jan 7, 2022

lance commented Dec 22, 2021 •

edited

Loading

grant left a comment •

edited

Loading