Use min max job runtime between class & global#437
Conversation
sambostock
requested review from
Mangara,
adrianna-chang-shopify and
simbasdad
Mangara
left a comment
Mangara
left a comment
There was a problem hiding this comment.
I like this approach much better.
adrianna-chang-shopify
left a comment
adrianna-chang-shopify
left a comment
There was a problem hiding this comment.
Thanks for tackling this, this is a lot easier to reason about 😄 I have one question about dropping the instance reader, but otherwise looks good! 👍
| return global_max unless class_max | ||
| return class_max unless global_max | ||
|
|
||
| [global_max, class_max].min |
There was a problem hiding this comment.
@Mangara I benchmarked this compared to compact, compact!, and nested conditionals. This is faster than both compact versions, and nearly identical to nested conditionals, so I changed it from our compact! version and went with this as I figure it's pretty clear, faster, and avoids allocating arrays entirely.
There was a problem hiding this comment.
Yep, that makes sense to me! I don't think performance is super critical here, as most iterations will contain some external calls (DB queries mostly) that will dominate the running time. But this is still very readable, so if we can get both performance and readability I won't say no 😄
The existing approach set `JobIteration.max_job_runtime` as the default value of the `job_iteration_max_job_runtime` `class_attribute`. However, this approach would close around the value at the time the `Iteration` module was included in the host `class`. This means that if a job class is defined before the host application sets `max_job_runtime`, the setting will not be honoured. For example, if using the `maintenance_tasks` gem, which defines a `TaskJob`, the default would be read when the gem is loaded, prior to initializers running, which is where the host application typically sets the global `max_job_runtime`. This commit changes the implementation in the following ways: - We no longer restrict the ability to increase the maximum in child classes. There were ways to get around the restriction, and there may be legitimate scenarios in which an application needs to allow an job to exceed its parent class' maximum job runtime, so we can simplify the implementation by removing the constraint. - We now take the minimum between the class attribute and the global maximum, which works around the default race, while still ensuring the application can enforce a maximum across all jobs, for stability. Co-authored-by: Sander Verdonschot <sander.verdonschot@shopify.com>
sambostock
force-pushed
the
min-max-job-runtime
branch
from
5faeb1d to
bb33191
Compare
3 participants
The existing approach set
JobIteration.max_job_runtimeas the default value of thejob_iteration_max_job_runtimeclass_attribute. However, this approach would close around the value at the time theIterationmodule was included in the hostclass.This means that if a job class is defined before the host application sets
max_job_runtime, the setting will not be honoured. For example, if using themaintenance_tasksgem, which defines aTaskJob, the default would be read when the gem is loaded, prior to initializers running, which is where the host application typically sets the globalmax_job_runtime.This commit changes the implementation in the following ways:
This is an alternative implementation to #436, with the goal of being simpler.
Closes #436