AfterCommitAsyncDispatcher does not pass event to custom scheduler

[grncdr]

Every layer from Broker->Scheduler passes a triple of (subscriber, event, event_record), but the very last step hides the event object and only passes the serializ(ed/able) record to custom schedulers:

rails_event_store/rails_event_store/lib/rails_event_store/after_commit_async_dispatcher.rb

Lines 9 to 11 in e7ede7d

	def call(subscriber, _, record)
	run { @scheduler.call(subscriber, record) }
	end

This is a problem for me because I'd like to run a predicate on the event in my custom scheduler before enqueueing a job. My specific use case involves a fairly frequent event, where only a tiny percentage are actually relevant to a given subscriber.

How I'm working around this

Monkey patch AfterCommitAsyncDispatcher in an initializer:

class RailsEventStore::AfterCommitAsyncDispatcher
  def call(subscriber, event, record)
    run { @scheduler.call(subscriber, event, record) } 
  end
end

Suggested patch

The existing API contract is obviously for 2 arguments only. I know arity checking is usually frowned upon, but I feel like it's an acceptable trade-off here:

class AfterCommitAsyncDispatcher
  def initialize(scheduler:)
    @scheduler = scheduler
    @scheduler_arity = scheduler.method(:call).arity
  end

  def call(subscriber, event, record)
    run { @scheduler_arity === 3 ? @scheduler.call(subscriber, event, record) : @scheduler.call(subscriber, record) } 
  end
end

---

What do you think? Should this be changed? Would the change be breaking or something that could happen in the 2.x series?

[mostlyobvious]

On the topic:

• passing the event instance for the purpose of making decisions — makes sense to me
• changing scheduler method arity — that's not going to happen in 2.x, but to be accepted in 3.x
• arity checking like any other conditional — acceptable hack if intended short term
  and for the greater good of smoother transition in next version; it would be ideal if we can decide on scheduler's call arity when initializing dispatcher

Aside: is there any reason why you'd monkeypatch RailsEventStore::AfterCommitAsyncDispatcher instead of passing your own project-specific dispatcher to RailsEventStore::Client initializer?

[grncdr]

Aside: is there any reason why you'd monkeypatch RailsEventStore::AfterCommitAsyncDispatcher instead of passing your own project-specific dispatcher to RailsEventStore::Client initializer?

Actually no 😅. I am passing a project-specific scheduler to the AfterCommitAsyncDispatcher, but I never considered subclassing the dispatcher. I guess I was hoping that the change would be upstreamed "soon" and I could delete my monkey patch. 🙃

[mostlyobvious]

Just ran into this while trying to introduce jitter (defined on event) into scheduler 🙃

[matthew-healy]

Hi folks! I ran into this exact problem today & ended up reimplementing AfterCommitAsyncDispatcher to instead delegate to a child dispatcher.

If the change to the scheduler API discussed here ends up in 3.0 that would work for my use-case, but I wonder if it might be possible to ship something before that by making this a purely additive change. I.e., adding something like an AfterCommitAsyncDelegatingDispatcher (name tbd) which behaves exactly like AfterCommitAsyncDispatcher, except that it calls a dispatcher rather than a scheduler?

The AfterCommitAsyncDispatcher would then just be an AfterCommitAsyncDelegatingDispatcher whose child dispatcher just wraps the scheduler interface & drops its event argument.

If this is a desirable change I'm happy to submit a PR. Just let me know.