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

Refactor "emit" to "persist" #119

Merged
merged 1 commit into from
Nov 13, 2023
Merged

Refactor "emit" to "persist" #119

merged 1 commit into from
Nov 13, 2023

Conversation

huntc
Copy link
Collaborator

@huntc huntc commented Nov 10, 2023

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

@huntc huntc added the enhancement New feature or request label Nov 10, 2023
@huntc huntc self-assigned this Nov 10, 2023
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.
Copy link
Member

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.

Copy link
Collaborator Author

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.

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

created issue #122

Copy link
Member

@patriknw patriknw left a 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

@huntc huntc merged commit 4aab7a9 into main Nov 13, 2023
1 check passed
@huntc huntc deleted the persist branch November 13, 2023 08:13
Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Persist vs emit Optional return values from marshaller
3 participants