-
Notifications
You must be signed in to change notification settings - Fork 43
Batching support #54
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
base: master
Are you sure you want to change the base?
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
In conjuction commanded/commanded#569 adds support for batching Ecto projections.