Batching support#54
Conversation
Added tests and relevant fixes to batch projector added after update callback test Update dependencies docs temporary commanded version Skip over partial seen batch remove elixir_uuid after rebase Return error on partially seen batch again
|
Note: review can wait until Calmwave has dogfooded this for a bit. |
drteeth
left a comment
drteeth
left a comment
There was a problem hiding this comment.
Thanks for the work on this feature. It looks great, well done.
As this PR relies on upstream changes to Commanded, it will have to wait until that work is merged. See commanded/commanded#569
Before calling the PR done, the reference to the calmwave fork would need to be dropped of course.
|
I'm curious what the status on this is. What are the experience in Calmwave, @cdegroot ? Also, I have a question: Let's say I set batch size to |
|
@anderslemke The subscription flushes on timeout (milliseconds) if fewer events are available, so batch_size: 100 won't wait for 99 more events. |
yordis
left a comment
yordis
left a comment
There was a problem hiding this comment.
I noticed a few issues with partial batch handling:
-
No event filtering - When a batch contains already-seen events (e.g., events [2,3,4] when watermark is 2), the user projection receives all events instead of just [3,4]. This can cause duplicate projections.
-
No locking - The watermark check and update aren't atomic. Two concurrent batches could both pass the check and cause race conditions.
-
Test verification - The test at lines 74-91 should verify that "e4" was actually projected, not just that the call succeeded.
The core issue is that apply(multi_fn, [multi]) at line 152 passes the multi but not the filtered events, so the user's lambda processes the original batch from closure scope.
A robust approach would be: lock → filter unseen events → update watermark → pass only unseen events to user projection.
6 participants
In conjuction commanded/commanded#569 adds support for batching Ecto projections.