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

feat: delete events #4

Merged
merged 6 commits into from
May 28, 2024
Merged

feat: delete events #4

merged 6 commits into from
May 28, 2024

Conversation

patriknw
Copy link
Member

  • following same structure as r2dbc plugin, which also has the grouping of deletes to a max number of events per statement
  • using TransactWriteItems, deletes must be on individual pk
  • delete marker to keep highest sequence number if all events have been deleted

DynamoDBJournalSpec is now successful

patriknw added 5 commits May 27, 2024 15:04
* following same structure as r2dbc plugin, which also has the
  grouping of deletes to a max number of events per statement
* using TransactWriteItems, deletes must be on individual pk
* delete marker to keep highest sequence number if all events have been deleted
@@ -41,27 +42,35 @@ import software.amazon.awssdk.services.dynamodb.model.QueryRequest
import JournalAttributes._
val req = QueryRequest.builder
.tableName(settings.journalTable)
.consistentRead(true)
Copy link
Member Author

Choose a reason for hiding this comment

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

I saw some flaky test fails in ci. Thought it was this, but maybe there is something more because another fail after changing this.

However, we need consistentReads for replay and seqNr queries. Seems default is false.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we'll need consistent reads. I wonder if the local in-memory DynamoDB creates replicas anyway?

We should probably start collecting test failures that need follow up. I'll try a few re-runs as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ran a few times and several similar errors. Always looks like eventually consistent reads. Eventually discovered this:

https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/DynamoDBLocal.UsageNotes.html#DynamoDBLocal.Differences

Read operations are eventually consistent. However, due to the speed of DynamoDB running on your computer, most reads appear to be strongly consistent.

So looks like dynamodb-local is always eventually consistent reads, ignoring the consistent reads flag? Not sure on its behaviour, but maybe we can just wait for writes to be seen first in the tests. And we'll probably want some regular testing against actual DynamoDB as well.

Copy link
Contributor

@pvlugter pvlugter left a comment

Choose a reason for hiding this comment

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

LGTM

Will try re-running tests a few times before merging.

@pvlugter pvlugter merged commit 56c2aa0 into main May 28, 2024
3 checks passed
@pvlugter pvlugter deleted the wip-delete-patriknw branch May 28, 2024 00:09
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