-
Notifications
You must be signed in to change notification settings - Fork 10
feat: Consolidate enqueue and enqueue_at to use enqueue_all under the hood #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 14 commits
ea21d7f
fa887ca
cd95fa9
d685eb9
3fdb8fa
42e7356
f83bf8a
ee687fb
f3bd29b
b635060
557f030
91a9471
289a2d6
dad4175
414d560
d18d3a4
a32acfa
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 |
|---|---|---|
|
|
@@ -7,26 +7,63 @@ def enqueue_after_transaction_commit? | |
| end | ||
|
|
||
| def enqueue(job) | ||
| _enqueue(job) | ||
| enqueue_all([job]) | ||
| job | ||
| end | ||
|
|
||
| def enqueue_at(job, timestamp) | ||
| _enqueue(job, run_at: Time.at(timestamp)) # rubocop:disable Rails/TimeZone | ||
| job.scheduled_at = Time.at(timestamp) # rubocop:disable Rails/TimeZone | ||
| enqueue_all([job]) | ||
| job | ||
| end | ||
|
|
||
| def enqueue_all(jobs) | ||
| return 0 if jobs.empty? | ||
|
|
||
| assert_safe_to_enqueue!(jobs) | ||
|
|
||
| Delayed.lifecycle.run_callbacks(:enqueue, jobs) do | ||
| now = Delayed::Job.db_time_now | ||
| rows = jobs.map { |job| build_insert_row(job, now) } | ||
| result = Delayed::Job.insert_all(rows) # rubocop:disable Rails/SkipsModelValidations | ||
| assign_provider_job_ids(jobs, result) if Delayed::Job.connection.supports_insert_returning? | ||
| end | ||
|
|
||
| mark_successfully_enqueued(jobs) | ||
| jobs.size | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def _enqueue(job, opts = {}) | ||
| if enqueue_after_transaction_commit_enabled?(job) | ||
| def assert_safe_to_enqueue!(jobs) | ||
| if jobs.any? { |job| enqueue_after_transaction_commit_enabled?(job) } | ||
| raise UnsafeEnqueueError, "The ':delayed' ActiveJob adapter is not compatible with enqueue_after_transaction_commit" | ||
| end | ||
| unless Delayed::Worker.delay_jobs == true | ||
| raise UnsafeEnqueueError, "The ':delayed' ActiveJob adapter is not compatible with delay_jobs false" | ||
| end | ||
|
Contributor
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. Not confident on this restriction, but removing it makes the code quite a bit messier. |
||
| end | ||
|
|
||
| opts.merge!({ queue: job.queue_name, priority: job.priority }.compact) | ||
| .merge!(job.provider_attributes || {}) | ||
| def assign_provider_job_ids(jobs, result) | ||
| ids = result.rows.map(&:first) | ||
| jobs.zip(ids) { |job, id| job.provider_job_id = id } | ||
| end | ||
|
|
||
| Delayed::Job.enqueue(JobWrapper.new(job), opts).tap do |dj| | ||
| job.provider_job_id = dj.id | ||
| end | ||
| def mark_successfully_enqueued(jobs) | ||
| jobs.each { |job| job.successfully_enqueued = true if job.respond_to?(:successfully_enqueued=) } | ||
|
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. Were you able to determine if this API is a necessary part of the contract?
Contributor
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 think it is sorta optional: rails/rails#41191. |
||
| end | ||
|
|
||
| def build_insert_row(job, now) | ||
| 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 | ||
| Delayed::Job.new(prepared).attributes.compact.merge('created_at' => now, 'updated_at' => now) | ||
| 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
Contributor
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_after_transaction_commit_enabled?(job) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ def enqueue(*args) | |
|
|
||
| def enqueue_job(options) | ||
| new(options).tap do |job| | ||
| Delayed.lifecycle.run_callbacks(:enqueue, job) do | ||
| Delayed.lifecycle.run_callbacks(:enqueue, [job]) do | ||
| job.hook(:enqueue) | ||
|
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. This
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. Technically I think we also deprecated hook methods like this, so maybe it's time to finally remove support. |
||
| Delayed::Worker.delay_job?(job) ? job.save : job.invoke_job | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,9 +2,9 @@ module Delayed | |
| module Plugins | ||
| class Instrumentation < Plugin | ||
| callbacks do |lifecycle| | ||
| lifecycle.around(:enqueue) do |job, *args, &block| | ||
|
CelticMajora marked this conversation as resolved.
|
||
| ActiveSupport::Notifications.instrument('delayed.job.enqueue', active_support_notifications_tags(job)) do | ||
| block.call(job, *args) | ||
| lifecycle.around(:enqueue) do |jobs, &block| | ||
| ActiveSupport::Notifications.instrument('delayed.job.enqueue', bulk_enqueue_tags(jobs)) do | ||
| block.call(jobs) | ||
| end | ||
| end | ||
|
|
||
|
|
@@ -34,6 +34,16 @@ def self.active_support_notifications_tags(job) | |
| job: job, | ||
| } | ||
| end | ||
|
|
||
| def self.bulk_enqueue_tags(jobs) | ||
|
CelticMajora marked this conversation as resolved.
|
||
| { | ||
| count: jobs.size, | ||
| table: Delayed::Job.table_name, | ||
|
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. The most accurate value here would be something like The reason I wouldn't assume it's always
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. In fact, it might make sense to emit counts by class and let the caller decide what to do with those class names, e.g.
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. @effron wdyt?
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 might also make sense to return IDs in this payload.
Contributor
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 think having a metric per class is valuable, and we can use that as a proxy to know that all jobs with the same class have the same table etc?
Contributor
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. or, |
||
| database: Delayed::Job.database_name, | ||
| database_adapter: Delayed::Job.database_adapter_name, | ||
|
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. Similar comment here w.r.t. relying on |
||
| jobs: jobs, | ||
| } | ||
| end | ||
| end | ||
| end | ||
| end | ||
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'm wondering if we should move this entire body into
Delayed::Backend::Baseto live alongsideenqueueandenqueue_job. (There are potentially further simplifications we could in turn make over there as well.)