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.

Sign up for GitHub

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

Jump to bottom

fix: DynamoDB - Avoid dropping sequence number progress #1260

Merged
merged 12 commits into from
Nov 22, 2024
Merged

fix: DynamoDB - Avoid dropping sequence number progress #1260

merged 12 commits into from
Nov 22, 2024

Conversation

leviramsey
Copy link
Contributor

@leviramsey leviramsey commented Nov 22, 2024 •
edited
Loading

The major fix here is retrying unprocessed batch-write items in the OffsetStoreDao: while the individual writes are atomic, the batch is not and reports partial success as a successful CompletionStage: it's on the caller to check that all items succeeded and retry ones that didn't.

@pvlugter
Copy link
Contributor

This fix was tested against a slightly under-provisioned offset table, to have some failed writes from throttling. Looks good.

pvlugter
pvlugter reviewed Nov 22, 2024
View reviewed changes
akka-projection-dynamodb/src/main/scala/akka/projection/dynamodb/internal/OffsetStoreDao.scala Outdated
unprocessedSlices.size,
unprocessedSlices.mkString(", "))

failed.asInstanceOf[Future[Done]] // safe, actually contains Nothing
Copy link
Contributor

Choose a reason for hiding this comment

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

Trying to cast an exception to a future.

java.lang.ClassCastException: class akka.projection.dynamodb.internal.OffsetStoreDao$BatchWriteFailed cannot be cast to class scala.concurrent.Future

@pvlugter
Copy link
Contributor

Applied some changes directly after review. Have tested again, and it looks better with the backoff delay on retries.

patriknw
patriknw approved these changes Nov 22, 2024
View reviewed changes
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, Great catch! I have a few minor code style comments, but will follow up with those in a separate PR.

@patriknw patriknw merged commit 17cd73d into akka:main Nov 22, 2024
21 of 22 checks passed
@patriknw patriknw added this to the 1.6.3 milestone Nov 22, 2024
@patriknw patriknw added the bug Something isn't working label Nov 22, 2024
This was referenced Nov 25, 2024
Merged
Closed
@leviramsey leviramsey deleted the offset-store-fixes branch November 28, 2024 16:06
@leviramsey
Copy link
Contributor Author

Refs: akka/akka-persistence-dynamodb#95

This was referenced Jan 6, 2025
Open
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pvlugter pvlugter pvlugter left review comments

@patriknw patriknw patriknw approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
1.6.3
Development

Successfully merging this pull request may close these issues.
3 participants
@leviramsey @pvlugter @patriknw