-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
Adding Job priority #24
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments
CREATE INDEX CONCURRENTLY IF NOT EXISTS queues_delayed_until_queue_name_idx | ||
ON swift_jobs.queues(delayed_until, queue_name) | ||
CREATE INDEX CONCURRENTLY IF NOT EXISTS queues_delayed_until_priority_queue_name_idx | ||
ON swift_jobs.queues(delayed_until, queue_name, priority ASC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which value takes the highest priority delayed_until
or priority
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which value takes the highest priority
delayed_until
orpriority
?
delayed_until
followed by priority
unfortunately, we don't have a tie breaker. Should two jobs have the same priority
and delayed_until
the first on the list will get picked first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that not mean priority is pretty much ignored as it would be a fairly rare occurrence two jobs get added at exactly the same time (remember delayed until is set to now if there is no delay)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that not mean priority is pretty much ignored as it would be a fairly rare occurrence two jobs get added at exactly the same time (remember delayed until is set to now if there is no delay)
You're always ahead of me :) I noticed the bug after providing the above answer. I refactored the index to priority
, delayed_until
, created_at
which is also the sorting order.
@@ -55,16 +55,36 @@ public final class PostgresJobQueue: JobQueueDriver { | |||
public struct JobOptions: JobOptionsProtocol { | |||
/// Delay running job until | |||
public var delayUntil: Date | |||
/// Priority for this jobs highest priority 0 to 9, lowest priority | |||
public var priority: Int16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this an enum? If five values were enough we could go for
enum Priority: UInt8 {
case lowest=0, lower, default, higher, highest
}
or something similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this an enum? If five values were enough we could go for
enum Priority: UInt8 { case lowest=0, lower, default, higher, highest }or something similar
This should work just fine, instead of 10 values, we'll have 5 and sort in descending order for priority
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking of this a little more, should this enum be exposed publicly or will I need to wrap it in a structure @adam-fowler ?
Need to figure out better ways to test this