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

Use recoverWith instead of recover in SnapshotSerializer akka backwards compatibility #841

Merged

Conversation

SakulK
Copy link
Contributor

@SakulK SakulK commented Dec 11, 2023

@pjfanning after taking another look I think this should make more sense, otherwise snapshotFromBinary can either return T or Try[T] (got bitten by AnyRef here..). I'm not sure why the previous version didn't blow up when testing this with real mongo and existing akka persistence snapshots.

@pjfanning
Copy link
Contributor

@SakulK have you tried out this change in an end to end use case? I would like to avoid making a large number of PRs here. I'm happy to change this but I would prefer to get it right this time around.

@SakulK
Copy link
Contributor Author

SakulK commented Dec 11, 2023

@pjfanning yes, this works for me end-to-end

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm

@pjfanning pjfanning merged commit a934df3 into apache:1.0.x Dec 11, 2023
17 of 18 checks passed
pjfanning pushed a commit to pjfanning/incubator-pekko that referenced this pull request Dec 11, 2023
@SakulK SakulK deleted the snapshot-serializer-akka-compatibility-fix branch December 11, 2023 11:19
pjfanning added a commit that referenced this pull request Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants