-
Notifications
You must be signed in to change notification settings - Fork 28
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
Conversation
47f70f6
to
778e8b7
Compare
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 |
c499af2
to
db7868f
Compare
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. |
Yes, it's also relatively easy to add. When going from Axon event -> cloud event we could have the default |
…rk with for non axon applications.
… are not valid extension names.
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.
Conceptually this looks fine to me. Just a bunch of JavaDoc comments, mainly.
...ing-boot-autoconfigure/src/main/java/org/axonframework/extensions/kafka/KafkaProperties.java
Show resolved
Hide resolved
...ing-boot-autoconfigure/src/main/java/org/axonframework/extensions/kafka/KafkaProperties.java
Outdated
Show resolved
Hide resolved
...axonframework/extensions/kafka/eventhandling/cloudevent/CloudEventKafkaMessageConverter.java
Outdated
Show resolved
Hide resolved
...axonframework/extensions/kafka/eventhandling/cloudevent/CloudEventKafkaMessageConverter.java
Outdated
Show resolved
Hide resolved
...axonframework/extensions/kafka/eventhandling/cloudevent/CloudEventKafkaMessageConverter.java
Show resolved
Hide resolved
...axonframework/extensions/kafka/eventhandling/cloudevent/CloudEventKafkaMessageConverter.java
Outdated
Show resolved
Hide resolved
...axonframework/extensions/kafka/eventhandling/cloudevent/CloudEventKafkaMessageConverter.java
Outdated
Show resolved
Hide resolved
...axonframework/extensions/kafka/eventhandling/cloudevent/CloudEventKafkaMessageConverter.java
Show resolved
Hide resolved
...axonframework/extensions/kafka/eventhandling/cloudevent/CloudEventKafkaMessageConverter.java
Show resolved
Hide resolved
...axonframework/extensions/kafka/eventhandling/cloudevent/CloudEventKafkaMessageConverter.java
Show resolved
Hide resolved
...rc/main/java/org/axonframework/extensions/kafka/eventhandling/cloudevent/ExtensionUtils.java
Outdated
Show resolved
Hide resolved
…e stored as, and retrieved as metadata on an Axon event.
2e5fa22
to
f5cae19
Compare
...axonframework/extensions/kafka/eventhandling/cloudevent/CloudEventKafkaMessageConverter.java
Outdated
Show resolved
Hide resolved
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.
Two suggestions remaining. One JavaDoc, one on the source field on the Cloud Event. Otherwise, I think we're there.
6aa68bf
to
92d53ee
Compare
Kudos, SonarCloud Quality Gate passed! |
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.
My concerns have been addressed, hence I'm approving this pull request.
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
Optionally:
[ ] Have a better way to deal with unsupported metadata like lists.