Add batching support #509
Add batching support #509
Conversation
- upcast test for handle_batch/2 - batch_reset_event_handler_test
There was a problem hiding this comment.
Thanks for the pull request @cdegroot it's a long requested feature. I've made a few minor comments or questions.
lib/commanded/event/handler.ex
Outdated
| defp handle_batch(events, context, %Handler{last_seen_event: last_seen_event} = state) do | ||
| %{event_number: last_event_number} = last_event = List.last(events) | ||
|
|
||
| if not is_nil(last_seen_event) and last_event_number <= last_seen_event do | ||
| Logger.debug(fn -> | ||
| describe(state) <> " has already seen event ##{inspect(last_event_number)}" | ||
| end) | ||
|
|
||
| confirm_receipt([last_event], state) | ||
| else | ||
| do_handle_batch(events, context, state) | ||
| end | ||
| end |
There was a problem hiding this comment.
The list of events can be filtered to remove any that have already been seen, then the remainder of the batch can be handled.
lib/commanded/event/handler.ex
Outdated
| In both cases, the error handler will be called. When just a reason is | ||
| returned, the assumption is that a system issue is preventing the process | ||
| from continuing and the error handler will be called with a `nil` event | ||
| argument. When an event and a reason are returned, the assumption is that | ||
| something is wrong with the event itself and therefore, it is passed in | ||
| as the first argument for the error handler. |
There was a problem hiding this comment.
Passing nil to the error callback function will break the existing contract. Could we pass the first event in the batch to prevent a breaking change and for simplicity?
There was a problem hiding this comment.
Looks like this was mismatched docs and implementation. Currently when no specific event is passed, we send the last event in the batch to allow skipping the entire batch. I've updated these docs with the assumption of that behaviour being ok. Otherwise we can update it to send the first event (and the docs accordingly)
| if Keyword.has_key?(config, :concurrency) and Keyword.has_key?(config, :batch_size) do | ||
| raise ArgumentError, | ||
| "both `:concurrency` and `:batch_size` are specified, this is not yet supported. Please choose one or the other." | ||
| end |
There was a problem hiding this comment.
Any reason why both of these options cannot be supported by Commanded? The eventstore will support batch size for multiple consumers and will attempt to distribute events appropriately.
There was a problem hiding this comment.
We don't use concurrency at the moment, and this is a bit of a scratch-your-own-itch PR, of course (as usual). Thinking through single-threaded batching was already complicated enough and we really want to dogfood code we submit, so we decided to make it a "not supported" thing for now. The "yet" is there to entice someone who does want/need it to fix that gap :)
lib/commanded/event/handler.ex
Outdated
| - `:skip` - skip the failed event by acknowledging receipt. Note that this is not | ||
| a valid return value in batching mode. |
There was a problem hiding this comment.
Is this comment still applicable, as it appears that :skip is supported for batch errors?
There was a problem hiding this comment.
Thanks for catching that, :skip is indeed supported, this just got missed as part of that. I've taken that bit out and clarified it's affects on batching
|
Apologies for the delay on addressing your comments @slashdotdash. They should all be more or less addressed now, let us know if there's anything else. |
This adds the option of having batched event handlers to speed things up. Please refer to the included documentation for details.
Related pull requests:
We (www.metrist.io) are running this in production with the following versions:
For comments spanning PRs, please use Slack