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

Allow configuring default priority value at midpoint of range #39

Merged
merged 10 commits into from
Apr 29, 2024

Conversation

tkoar
Copy link
Contributor

@tkoar tkoar commented Apr 3, 2024

Summary

This PR Adds a configuration value (.assign_at_midpoint) to Delayed::Priority

The intent is to allow users to change the default priority of a job (i.e. when using a named priority) to be the middle of the configured priority ranges -- derived from the default or custom priority names -- as opposed to the starting value of the range.

By allowing a default value that is in the middle of the range, users will be able to bump the priority of some jobs such that they remain in their assigned queue but take priority over jobs of the same named priority.

Note: since the last priority uses a range that is infinite, it's not possible to derive the midpoint. So, for now, I've defaulted to adding 5, since that matches our defaulted priority values.

…ed priorities to a midpoint-within-range value'
@tkoar tkoar self-assigned this Apr 3, 2024
lib/delayed/priority.rb Outdated Show resolved Hide resolved
lib/delayed/priority.rb Outdated Show resolved Hide resolved
lib/delayed/priority.rb Outdated Show resolved Hide resolved
@tkoar tkoar force-pushed the tk/allow-mid-point-priority-assignment branch from 025ff35 to fcc4b9a Compare April 3, 2024 21:25
lib/delayed/priority.rb Outdated Show resolved Hide resolved
lib/delayed/priority.rb Outdated Show resolved Hide resolved
Comment on lines +503 to +510
Job priorities can specified by using the name of the desired range (i.e. :user_visible).
By default, the value for a named priority will be the first value in that range.
To set each priority's default value to the middle of its range (i.e. 15 for :user_visible), Delayed::Priority can be configured with:

```ruby
Delayed::Priority.assign_at_midpoint = true
```

Copy link
Member

@smudge smudge Apr 5, 2024

Choose a reason for hiding this comment

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

Yup, this makes sense. At some point we should consider making this the default, but opt-in is good for now.

Comment on lines 176 to 182
def -(other)
to_i - other.to_i
end

def +(other)
to_i + other.to_i
end
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about what it meant to always call to_i on other, and also what it means to always respond with an integer. For example:

Priority.new(1) + Priority.new(10)

I would expect that to return Priority.new(11), not 11.

And also these examples:

Priority.new(10) + 1

Should also probably return Priority.new(11).

But I'm not sure what this should return:

Priority.new(10) + 0.9

(Should it round to Priority.new(11), or raise an error?)

Maybe we don't need to handle that last case for now.

So I think that leaves us with this:

Suggested change
def -(other)
to_i - other.to_i
end
def +(other)
to_i + other.to_i
end
def -(other)
new(to_i - other.to_i)
end
def +(other)
new(to_i + other.to_i)
end

Though we could do what we do in <=> and cast other to_i only if it's a priority:

other = other.to_i if other.is_a?(self.class)

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah that's a good call! We should return a Priority instance, since that is the caller.

Hmm yeah the math complexities are interesting. I would say we probably kick the can on dealing with floats? Right now the initialize method (and the mid_point method) will ensure all instances have integer values.

Copy link
Member

@smudge smudge left a comment

Choose a reason for hiding this comment

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

domain LGTM && platform LGTM

@smudge smudge merged commit 24b75f5 into Betterment:main Apr 29, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants