-
Notifications
You must be signed in to change notification settings - Fork 1
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
Refactor "emit" to "persist" #119
Conversation
Refactors "emit" as "persist" and attempts to convey the intention of associated operations that persistence is required. As a consequence, the commit log marshaller is refactored to remove the optionality of some return types to support that all events must be persisted. Along the way, I also refactored some options into a result with increased error reporting and logging.
|
||
/// Extract an entity id from a consumer envelope. | ||
fn to_entity_id(&self, record: &ConsumerRecord) -> Option<EntityId>; | ||
|
||
/// Produce an event envelope from a consumer record. | ||
/// Note that this may not always be possible due to record formats having | ||
/// changed, in which case we want the consumer to continue and skip it. | ||
/// Changes in a record's layout should not prevent the system from working. |
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.
This looks like a difference from Akka JVM. There we don't automatically accept and ignore events with invalid format. The reason is that it is most likely a mistake to not evolve the serialization format in a compatible way, and then ignoring such events and continue with other events can lead to data corruption on the consumer side.
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’m definitely relying on this behaviour with my production system. Given that events can persist in a commit long, potentially indefinitely, it’s a handy feature.
What would Akka do when sourcing events from Kafka? It seems wrong for the app to not be able to recover.
Would you mind creating a separate issue for this so we can move forward? Thanks.
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.
It should be a conscious application decision by the application to skip events that it can't deserialize. For example an application specific serializer implementation that makes that decision.
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.
created issue #122
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.
LGTM, with separate follow up to #122
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.
LGTM
Refactors "emit" as "persist" and attempts to convey the intention of associated operations that persistence is required.
As a consequence, the commit log marshaller is refactored to remove the optionality of some return types to support that all events must be persisted. Along the way, I also refactored some options into a result with increased error reporting and logging.
Fixes #94
Fixes #96
Downstream: https://github.com/lightbend/akka-projection-temp/pull/19