feat: DynamoDB projections #1222
Conversation
pvlugter
force-pushed
the
dynamodb
branch
2 times, most recently
from
5441174 to
b202914
Compare
| @@ -38,6 +38,13 @@ object Dependencies { | |||
| case Seq(major, minor, _*) => s"$major.$minor" | |||
| } | |||
|
|
|||
| val AkkaPersistenceDynamodb = "2.0.0-M1" | |||
There was a problem hiding this comment.
First 2.0 milestone published, while depending on Akka 2.10 milestones.
| private def readTimestampOffset(): Future[TimestampOffsetBySlice] = { | ||
| val oldState = state.get() | ||
| // retrieve latest timestamp for each slice, and use the earliest | ||
| val futTimestamps = |
There was a problem hiding this comment.
This is conceivably 1024 queries fired off right at startup, wonder if it's better to do this in a stream with mapAsync and limited parallelism?
There was a problem hiding this comment.
Yes, needing the separate queries is a weak point for the plugin. The events-by-slices queries are also all separate and in parallel. How many queries that actually run in parallel should come down to the concurrency settings for the DynamoDB client. Not sure if @patriknw had any other thoughts at the time?
(And let's do any improvements in separate PRs. With this one as the initial copy over.)
There was a problem hiding this comment.
Is the concern that it creates a burst that is not desired? I don't know if that is a problem. Create an issue/pr if that has been seen as a real problem when testing.
events-by-slices queries are more separate and therefore more spread over time
There was a problem hiding this comment.
At minimum, if client.http.max-concurrency + client.http.max-pending-connection-acquires is less than 1024 (which particularly latency-sensitive applications may end up having), there is no chance of a DynamoDB projection successfully starting, so the projection should check for this.
Another possibility would be having the queries here retry, so that the offsets can eventually be recovered.
There was a problem hiding this comment.
I'll have a PR up for at least one of these when this is merged
There was a problem hiding this comment.
sounds good to add the bounded parallelism, thanks
Ported over from the internal incubating project.