-
Notifications
You must be signed in to change notification settings - Fork 10
feat: Introduce enqueue_all support as part of the Rails 7.1 perform_all_later method. #103
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: main
Are you sure you want to change the base?
Changes from all commits
ea21d7f
fa887ca
cd95fa9
d685eb9
3fdb8fa
42e7356
f83bf8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,8 +14,82 @@ def enqueue_at(job, timestamp) | |
| _enqueue(job, run_at: Time.at(timestamp)) # rubocop:disable Rails/TimeZone | ||
| end | ||
|
|
||
| # `perform_all_later` / `enqueue_all` is an ActiveJob 7.1+ feature. | ||
| # Earlier ActiveJob versions lack both the caller (`perform_all_later`) | ||
| # and the per-job `successfully_enqueued=` setter, so we gate only the | ||
| # public method — the private helpers are unconditional and just unused | ||
| # on <7.1. | ||
| if ActiveJob.gem_version >= Gem::Version.new('7.1') | ||
| def enqueue_all(jobs) | ||
| return 0 if jobs.empty? | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not clear to me if
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Other adapters I was looking at(ie: Solid Queue and Sidekiq) all returned the count, but it was unclear whether that was part of the contract. |
||
|
|
||
| jobs.each do |job| | ||
| if enqueue_after_transaction_commit_enabled?(job) | ||
| raise UnsafeEnqueueError, "The ':delayed' ActiveJob adapter is not compatible with enqueue_after_transaction_commit" | ||
| end | ||
| end | ||
|
|
||
| # Fall back to the per-job path when we can't take the bulk shortcut. | ||
| return enqueue_all_one_by_one(jobs) unless bulk_enqueue_supported? | ||
|
|
||
| rows = jobs.map { |job| build_insert_row(job) } | ||
| result = Delayed::Job.insert_all(rows, record_timestamps: true) # rubocop:disable Rails/SkipsModelValidations | ||
| ids = result.rows.map(&:first) | ||
|
|
||
| jobs.zip(ids) do |job, id| | ||
| job.provider_job_id = id | ||
| job.successfully_enqueued = true | ||
|
Comment on lines
+40
to
+41
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Setting Additionally, the way that My thinking is that we could simplify this method significantly (and support MySQL) if we skip handling (IMO, if we actually wanted to set it in the ActiveJob payload, we should generate a |
||
| end | ||
|
|
||
| ids.size | ||
| end | ||
| end | ||
|
|
||
| private | ||
|
|
||
| # Bulk enqueue needs to return the new IDs so we can populate | ||
| # `provider_job_id` on each input job. That requires both: | ||
| # - delay_jobs = true (otherwise each job runs inline via `save`/`invoke_job`) | ||
| # - an adapter that supports INSERT ... RETURNING (MySQL does not) | ||
| def bulk_enqueue_supported? | ||
| Delayed::Worker.delay_jobs == true && Delayed::Job.connection.supports_insert_returning? | ||
| end | ||
|
|
||
| def build_insert_row(job) | ||
| opts = { queue: job.queue_name, priority: job.priority }.compact | ||
| opts.merge!(job.provider_attributes || {}) | ||
| opts[:run_at] = coerce_scheduled_at(job.scheduled_at) if job.scheduled_at | ||
|
|
||
| prepared = Delayed::Backend::JobPreparer.new(JobWrapper.new(job), opts).prepare | ||
| dj = Delayed::Job.new(prepared) | ||
|
|
||
| Delayed.lifecycle.run_callbacks(:enqueue, dj) do | ||
| dj.hook(:enqueue) | ||
| end | ||
|
|
||
| # Replicate `before_save` hooks since insert_all bypasses callbacks. | ||
| dj.before_save_hooks | ||
| dj.attributes.compact | ||
| end | ||
|
|
||
| def coerce_scheduled_at(value) | ||
| value.is_a?(Numeric) ? Time.at(value) : value # rubocop:disable Rails/TimeZone | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't sure if we needed to support old versions of ActiveJob where it was a numeric: https://apidock.com/rails/ActiveJob/Core/scheduled_at%3D
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, looks like that landed in 7.1, so we'd need to restrict our I think we can consider changing support in a separate PR and keep the coercion in place here, if that makes sense to you. |
||
| end | ||
|
|
||
| def enqueue_all_one_by_one(jobs) | ||
| # Wrap in a transaction so a mid-loop failure (e.g. StaleEnqueueError on | ||
| # job N) rolls back jobs 0..N-1, matching the bulk path's all-or-nothing | ||
| # semantics. | ||
| Delayed::Job.transaction do | ||
| jobs.count do |job| | ||
| opts = job.scheduled_at ? { run_at: coerce_scheduled_at(job.scheduled_at) } : {} | ||
| _enqueue(job, opts) | ||
| job.successfully_enqueued = true | ||
| true | ||
| end | ||
| end | ||
|
Comment on lines
+80
to
+90
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may be better off
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be preferred to have this delegate as if
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I follow. I think we should allow |
||
| end | ||
|
|
||
| def _enqueue(job, opts = {}) | ||
| if enqueue_after_transaction_commit_enabled?(job) | ||
| raise UnsafeEnqueueError, "The ':delayed' ActiveJob adapter is not compatible with enqueue_after_transaction_commit" | ||
|
|
||
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.
It would be nice to avoid expanding the public API of the Job model -- I understand wanting to avoid drift, but it also doesn't stop someone from adding a
before_saveand skipping callbacks again.My thinking is that these should not be
before_savecalls at all and instead should be handled by theenqueueandenqueue_allmethods. We could retain validations on this class to ensure that they are always set by the caller, but remove some of the magic so that the caller is always forced to call these two methods (or supply their own values) before saving.Uh oh!
There was an error while loading. Please reload this page.
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.
Actually instead of validations, for our purposes, ensuring that we have appropriate DB constraints would be enough. (Otherwise we'd need to call
validate!on each job while constructing the insert all payload, and that might not be worth the minor performance hit if we can get the guarantee from the DB itself.)