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

Lookup global max_job_runtime at runtime #436

Closed
wants to merge 1 commit into from
Closed

Lookup global max_job_runtime at runtime #436

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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

### Bug fixes

- [436](https://github.com/Shopify/job-iteration/pull/436) - Defer reading `JobIteration.max_job_runtime` until runtime, instead of closing around the value at the time of job definition.
- [431](https://github.com/Shopify/job-iteration/pull/431) - Use `#id_value` instead of `send(:id)`
when generating position for cursor based on `:id` column (Rails 7.1 and above, where composite
primary models are now supported). This ensures we grab the value of the id column, rather than a
Expand Down
12 changes: 9 additions & 3 deletions lib/job-iteration/iteration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def inspected_cursor
end
end

included do |_base|
included do |base|
define_callbacks :start
define_callbacks :shutdown
define_callbacks :complete
Expand All @@ -49,10 +49,16 @@ def inspected_cursor
:job_iteration_max_job_runtime,
instance_writer: false,
instance_predicate: false,
default: JobIteration.max_job_runtime,
)

singleton_class.prepend(PrependedClassMethods)
class << base
prepend(PrependedClassMethods)

def job_iteration_max_job_runtime
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm missing something about this. Wouldn't it always return the default, instead of the override?

Why define this method here, rather than with the other methods added by this class, such as job_should_exit??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's doing on here is that we're using class_attribute, and the way it implements "inheritable attributes" is that when you invoke the setter method, it redefines the method on the receiver.

So stepping through it, this is more or less what happens:

class ParentJob < ActiveJob::Base
  include JobIteration::Iteration
end

The include causes the included block to run which first invokes

class ParentJob < ActiveJob::Base
  class_attribute(
    :job_iteration_max_job_runtime,
    instance_writer: false,
    instance_predicate: false,
  )
end

which basically does

class ParentJob < ActiveJob::Base
  class << self
    def job_iteration_max_job_runtime
      nil # or whatever default is passed in
    end
    
    def job_iteration_max_job_runtime=(value)
      # ...redefine job_iteration_max_job_runtime to return value...
    end
  end
end

Because there's no way to pass in a dynamic default (we want it to delegate to JobIteration.max_job_runtime, not use the value at the time the method was defined; that's the bug), the only way to do that is to define the method ourselves, which is what this new part of included does

class ParentJob < ActiveJob::Base
  class << self
    def job_iteration_max_job_runtime
      JobIteration.max_job_runtime
    end
  end
end

From there, if a consumer invokes the setter method,

class ChildJob < ParentJob
  self.job_iteration_max_job_runtime = 1.minute
end

it will redefine the method, "shadowing" the parent one

class ChildJob < ParentJob
  class << self
    def job_iteration_max_job_runtime
      1.minute
    end
  end
end

The semantics of class_attribute match what we want, it just doesn't allow for a dynamic default, hence why we have to do this.

We can't just put the method in ClassMethods, because the method defined by class_attributes would shadow it, but it does occur to me we could put it in PrependedClassMethods, which should shadow the class_attributes implementation, while still allowing consumers to override it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think part of my confusion comes from class_attribute adding both class and instance accessor methods. This implementation only overrides one of them, but not the other, right?

# Lookup default every time, instead of closing around its value when the class is defined.
JobIteration.max_job_runtime
end
Comment on lines +57 to +60
Copy link

Choose a reason for hiding this comment

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

If I'm reading this code a month from now, I'm not sure it would be clear to me why we're doing this.

What is the advantage of this method vs doing 'job_iteration_max_job_runtime || JobIteration.max_job_runtimeinjob_should_exit?`

You could even do [job_iteration_max_job_runtime, JobIteration.max_job_runtime].compact.min to enforce that a job can't override the global default.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, I feel like this simplifies the logic, versus having to worry about whether the job-level config is shadowing the global one properly. I don't see that we explicitly mentioned anywhere that job_iteration_max_job_runtime defaults to the value of JobIteration.max_job_runtime, so I don't think changing the way this works would be a breaking change.

end
end

module PrependedClassMethods
Expand Down
11 changes: 11 additions & 0 deletions test/unit/iteration_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,17 @@ def test_global_max_job_runtime
end
end

def test_global_max_job_runtime_with_updated_value
freeze_time
with_global_max_job_runtime(10.minutes) do
klass = build_slow_job_class(iterations: 3, iteration_duration: 30.seconds)
with_global_max_job_runtime(1.minute) do
klass.perform_now
assert_partially_completed_job(cursor_position: 2)
end
end
end

def test_per_class_max_job_runtime_with_default_global
freeze_time
parent = build_slow_job_class(iterations: 3, iteration_duration: 30.seconds)
Expand Down