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: Initial journal implementation #2

Merged
merged 1 commit into from
May 27, 2024
Merged

Conversation

patriknw
Copy link
Member

  • write and replay events
  • client provider
  • config structure

* write and replay events
* client provider
* config structure
}
}

class DynamoDBJournalSpec extends JournalSpec(DynamoDBJournalSpec.config) with TestDbLifecycle {
Copy link
Member Author

Choose a reason for hiding this comment

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

Some tests (related to deletes) will fail because that isn't implemented yet. I think we can merge this anyway after review, and add that in separate pr.

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.

Good to see the initial journal implementation.

Tried it out locally, and while looking also made some fixes directly — can push separately.

CI tests are not bringing up the local DynamoDB. Looks like the docker compose version thing that we've seen before (docker-compose vs docker compose). Can check in another PR.


existingTable.transformWith {
case Success(_) => delete().flatMap(_ => create())
case Failure(_: ResourceNotFoundException) => create()
Copy link
Contributor

Choose a reason for hiding this comment

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

When trying locally it's not creating the table. Because this is wrapped in a future CompletionException.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added case for CompletionException in #3.

import JournalAttributes._
val req = QueryRequest.builder
.tableName(settings.journalTable)
.keyConditionExpression(s"$Pid = :pid AND $SeqNr BETWEEN :from AND :to")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is failing on one of the tests when running locally.

must not replay messages if count limit equals 0
Invalid KeyConditionExpression: The BETWEEN operator requires upper bound to be greater than or equal to lower bound

The logic copied from r2dbc plugin has to < from when max/limit is 0.

Could be a check in this method, return empty source.

Copy link
Contributor

Choose a reason for hiding this comment

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

Checked this in #3.

Comment on lines +219 to +221
// FIXME very inefficient implementation, can we get rid of the separate ReadHighestSequenceNr query
// altogether and rely on latest event in the replay? Requires some changes in Akka, but would
// be good performance improvement.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can have a more efficient query for DynamoDB now as well — should be possible to get the last item for a partition key, which would be the highest sequence number.

Copy link
Contributor

@pvlugter pvlugter May 27, 2024

Choose a reason for hiding this comment

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

Added a last item query (backwards scan with limit of 1) to get highest sequence number directly, in #3.


The following client settings be overridden in the `client` block:

@@snip [reference.conf](/core/src/main/resources/reference.conf) {#client-settings}
Copy link
Contributor

Choose a reason for hiding this comment

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

Failing docs in CI, with Label [client-settings] block not closed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added the missing label in #3.

@pvlugter
Copy link
Contributor

CI tests are not bringing up the local DynamoDB. Looks like the docker compose version thing that we've seen before (docker-compose vs docker compose). Can check in another PR.

Fixed in #3.

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.

We could merge and then follow up with changes in #3.

@patriknw patriknw merged commit e89efb0 into main May 27, 2024
1 of 3 checks passed
@patriknw patriknw deleted the wip-journal0-patriknw branch May 27, 2024 10:50
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