Skip to content
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.

Sign up for GitHub

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

Ensure that maintenance tasks have a default max runtime #918

Closed
wants to merge 1 commit into from
Closed

Ensure that maintenance tasks have a default max runtime #918

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/jobs/concerns/maintenance_tasks/task_job_concern.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ module TaskJobConcern
include JobIteration::Iteration

included do
self.job_iteration_max_job_runtime = JobIteration.max_job_runtime || 5.minutes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will still result in us reading the config at the time the concern is included, which may be before the host application sets the value. I think it would make sense to make it dynamic by using a method instead.

-    included do
+    included do |base|
Suggested change
self.job_iteration_max_job_runtime = JobIteration.max_job_runtime || 5.minutes
class << base
def job_iteration_max_job_runtime = JobIteration.max_job_runtime || 5.minutes
end

although maybe that could be defined in class_methods instead... 🤔

From there, I think it would make sense to deprecate, and eventually remove

# lib/maintenance_tasks/engine.rb
config.after_initialize do
  JobIteration.max_job_runtime ||= 5.minutes

because really this gem shouldn't be setting a global config affecting other jobs (acknowledging the per-job option didn't exist at the time that was added).

We could also make that 5.minutes configurable as MaintenanceTasks.max_job_runtime, if we wanted, and just have 5.minutes as the default, but that's not a big deal. 🤷

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let's wait for Shopify/job-iteration#436 to ship, fixing in job-iteration is more correct anyways. I'd still prefer to see the default configured in our engine though (could be in an initializer block instead of after_initialize though, as discussed).

because really this gem shouldn't be setting a global config affecting other jobs

Completely agree. Initially I was wondering if we'd even need to deprecate it, but I guess you could end up with a change in behaviour: if you're using JobIteration.max_job_runtime to control the value, and we switch to using the class-level config, then our default could get picked up instead of the configured value in the app.

Deprecating might be tricky though, would we warn if the global writer is used and tell people to switch to using the class-level config on MaintenanceTasks::TaskJob?


before_perform(:before_perform)

on_start(:on_start)
Expand Down
4 changes: 4 additions & 0 deletions test/jobs/maintenance_tasks/task_job_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,5 +617,9 @@ class << self

assert_equal 2, @run.reload.tick_total
end

test "job_iteration_max_job_runtime defaults to 5 minutes" do
assert_equal 5.minutes, MaintenanceTasks::TaskJob.job_iteration_max_job_runtime
end
end
end