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

Add Cloud event serializer #321

Merged
merged 9 commits into from Sep 15, 2022
Merged

Add Cloud event serializer #321

merged 9 commits into from Sep 15, 2022

Conversation

gklijs
Copy link
Collaborator

@gklijs gklijs commented Aug 19, 2022

Having a cloud event serializer would make it easier to use this extension when there is an existing Kafka cluster used, where the chosen format on the wire is Cloud Events. For example to build a projection combining 'external' events from Kafka with 'internal' events from Axon Server, or to send certain events to Kafka for 'external' use.

Some things left to do

  • Support mapping metadata key to extension name conversion, extension names can only use lowercase letters and digits. My idea is to add an optional String, String map to the builder, to do the conversion.
  • Add autowire support, when the Cloudevent serializer is used, when should also change the Kafka config so the correct Kafka Serializer and Deserializer are used.

Optionally:

  • Add an example project using cloud events, or add to the current one.
  • [ ] Have a better way to deal with unsupported metadata like lists.

@gklijs gklijs added this to the Release 4.6.0 milestone Aug 19, 2022
@gklijs gklijs self-assigned this Aug 19, 2022
@gklijs gklijs force-pushed the feature/add-cloudeventserializer branch 4 times, most recently from 47f70f6 to 778e8b7 Compare August 31, 2022 15:18
@gklijs
Copy link
Collaborator Author

gklijs commented Sep 1, 2022

I don't think we should/could take on the remaining point. I don't expect people to put Lists in the metadata, and it's something that's currently 'broken' for the DefaultKafkaMessageConverter. Some additional thing I'm not 100% happy with is that the dataContentType and dataSchema or lost when set, when converted to an event. I think that's fine as we don't have anything similar in Axon, although we could just put them in metadata.

@gklijs gklijs marked this pull request as ready for review September 1, 2022 10:04
@gklijs gklijs force-pushed the feature/add-cloudeventserializer branch from c499af2 to db7868f Compare September 1, 2022 15:32
@smcvb smcvb requested review from a team, CodeDrivenMitch, smcvb and YvonneCeelie and removed request for a team September 12, 2022 13:26
@smcvb
Copy link
Member

smcvb commented Sep 13, 2022

I don't think we should/could take on the remaining point. I don't expect people to put Lists in the metadata, and it's something that's currently 'broken' for the DefaultKafkaMessageConverter. Some additional thing I'm not 100% happy with is that the dataContentType and dataSchema or lost when set, when converted to an event. I think that's fine as we don't have anything similar in Axon, although we could just put them in metadata.

I think there's benefit on leaving that in the metadata. Assuming it's necessary to move back from Axon's Event format to Cloud Events, of course.

@gklijs
Copy link
Collaborator Author

gklijs commented Sep 13, 2022

Yes, it's also relatively easy to add. When going from Axon event -> cloud event we could have the default dataContentTypeSupplier and dataSchemaProvider get it from the metadata.

Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

Conceptually this looks fine to me. Just a bunch of JavaDoc comments, mainly.

…e stored as, and retrieved as metadata on an Axon event.
@gklijs gklijs force-pushed the feature/add-cloudeventserializer branch 5 times, most recently from 2e5fa22 to f5cae19 Compare September 13, 2022 14:47
Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

Two suggestions remaining. One JavaDoc, one on the source field on the Cloud Event. Otherwise, I think we're there.

@gklijs gklijs force-pushed the feature/add-cloudeventserializer branch from 6aa68bf to 92d53ee Compare September 14, 2022 15:26
@sonarcloud
Copy link

sonarcloud bot commented Sep 15, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

92.7% 92.7% Coverage
0.0% 0.0% Duplication

Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

My concerns have been addressed, hence I'm approving this pull request.

@smcvb smcvb merged commit a12b98b into master Sep 15, 2022
@smcvb smcvb deleted the feature/add-cloudeventserializer branch September 15, 2022 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants