-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add JsonbSerde to Kafka client module #2880
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made a few minor comments.
But it looks pretty good!
extensions/kafka-client/runtime/src/main/java/io/quarkus/kafka/client/serde/JsonbSerde.java
Outdated
Show resolved
Hide resolved
extensions/kafka-client/runtime/src/main/java/io/quarkus/kafka/client/serde/JsonbSerde.java
Outdated
Show resolved
Hide resolved
extensions/kafka-client/runtime/src/main/java/io/quarkus/kafka/client/serde/JsonbSerde.java
Outdated
Show resolved
Hide resolved
integration-tests/kafka/src/test/java/io/quarkus/it/kafka/streams/KafkaStreamsTest.java
Show resolved
Hide resolved
81bf2f6
to
fef7829
Compare
@cescoffier, reworked the commit to address your comment regarding the naming. It's now |
Waiting for CI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, one last comment.
...ons/kafka-client/runtime/src/main/java/io/quarkus/kafka/client/serialization/JsonbSerde.java
Outdated
Show resolved
Hide resolved
...ons/kafka-client/runtime/src/main/java/io/quarkus/kafka/client/serialization/JsonbSerde.java
Outdated
Show resolved
Hide resolved
...afka-client/runtime/src/main/java/io/quarkus/kafka/client/serialization/JsonbSerializer.java
Show resolved
Hide resolved
@cescoffier, Next try, sent an update. How hard can it be :) |
Thanks @gunnarmorling, look great now. Even thinking making is a noteworthy-feature. |
@gsmet, @cescoffier, here's a follow-up to the Kafka Streams work which should ideally go into the next release: it adds the JSON-B based SerDe ready for re-use to the runtime module, as suggested by Clement. It will simplify the quickstart code.
Note I've removed the JSON-P based SerDe which existed in the tests before for the sake of the new one. We also could consider to add this one ready-made, too, but it doesn't strike as super-important to me.
See #2663