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.

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

Use min max job runtime between class & global #437

Merged
merged 1 commit into from
Nov 16, 2023
Merged

Conversation

sambostock
Copy link
Contributor

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.

This is an alternative implementation to #436, with the goal of being simpler.


Closes #436

@sambostock sambostock added the bug Something isn't working label Nov 15, 2023
Copy link
Contributor

@Mangara Mangara left a comment

Choose a reason for hiding this comment

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

I like this approach much better.

Copy link
Contributor

@adrianna-chang-shopify adrianna-chang-shopify left a comment

Choose a reason for hiding this comment

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

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! 👍

lib/job-iteration/iteration.rb Show resolved Hide resolved
Comment on lines +283 to +286
return global_max unless class_max
return class_max unless global_max

[global_max, class_max].min
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 😄

Copy link
Contributor

@adrianna-chang-shopify adrianna-chang-shopify left a comment

Choose a reason for hiding this comment

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

🎉

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 sambostock merged commit 5464da0 into main Nov 16, 2023
39 checks passed
@sambostock sambostock deleted the min-max-job-runtime branch November 16, 2023 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants