-
Notifications
You must be signed in to change notification settings - Fork 3.6k
chore: don't fail deserialization on unknown event metadata #32812
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
base: main
Are you sure you want to change the base?
Conversation
|
Could be that we need more tuning, logging or opt in/out |
patriknw
left a comment
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.
looking good
| serialization | ||
| .deserialize(persistentPayload.getPayload.toByteArray, persistentPayload.getSerializerId, manifest) | ||
| .get | ||
| .toOption |
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.
Shall we have a marker trait to define that the metadata is mandatory and should fail deserialization instead of skipping? Then we have it as default to ignore, but can mark certain things, like ReplicatedEventMetadata.
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.
We only have a serializer id and a manifest, no class on the deserializing side when unknown metadata type so we couldn't know that the marker is applied?
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.
yeah, didn't think (that far)
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 was thinking that it could be only for unknown serializer id:s but where we hit it ourselves was there is an existing serializer for protos but the message type is unknown, so that doesn't work well enough either.
|
Was thinking about logging as a warning on first occurrence per serializer id + manifest, and still ignore, but that wouldn't help our SDK users defining custom metadata, warning would go in runtime logs. |
|
New idea, probably with a toggle to enable, we could collect such failures in a special kind of public API metadata entry, that it is then possible to look for in the events and deal with in a suitable way in a given application. |
References: