refactor: Remove 'ready_scope' and improve remaining scopes. #63
Conversation
The goal is to create some unifying structure around the way that scopes are defined and organized in the ActiveRecord `Job` class, without changing the actual SQL in any meaningful way. (This sets me up to make meaningful SQL changes in future PRs!) In addition to using `Arel` for `gt`/`lt`/`gteq`/`lteq`operations, I renamed a few concepts: - `not_failed` => `live` - `locked` => `claimed` - `not_locked` => `not_claimed` I also added two new scopes: - `future` (where run_at > now) - `pending` (where run_at <= now) Again, this should **not** meaningfully change any SQL! Only minor formatting changes without changing the meaning of each query. (I'm using the tests to prove this.)
Pay close attention to parenthesis and operator precedence. (The actual logic should not be changing.)
| WHERE (((run_at <= '2025-11-10 17:20:13' AND (locked_at IS NULL OR locked_at < '2025-11-10 16:59:43')) OR locked_by = 'worker1') | ||
| AND failed_at IS NULL) | ||
| WHERE ("delayed_jobs"."run_at" <= '2025-11-10 17:20:13' | ||
| AND ("delayed_jobs"."locked_at" IS NULL OR "delayed_jobs"."locked_at" < '2025-11-10 16:59:43') | ||
| OR "delayed_jobs"."locked_by" = 'worker1') |
There was a problem hiding this comment.
(((A <= v1 AND (B IS NULL OR B < v2)) OR C = v3) AND D IS NULL)
vs
(A <= v1 AND (B IS NULL OR B < v2) OR C = v3) AND D IS NULL
Could use someone to spot-check the parenthesis and operator precedence here, but I think these are logically identical 😅
effron
left a comment
effron
left a comment
There was a problem hiding this comment.
just some nonblocking thoughts/questions. domainLGTM!
| scope :not_failed, -> { where(failed_at: nil) } | ||
| scope :workable, ->(timestamp) { not_locked.not_failed.where("run_at <= ?", timestamp) } | ||
| scope :working, -> { locked.not_failed } | ||
| scope :erroring, -> { where.not(last_error: nil) } |
There was a problem hiding this comment.
this probably wants to exclude failed jobs, right? if not, maybe it needs a more inclusive name? errored?
There was a problem hiding this comment.
Indeed -- I wanted to refactor/reorganize the file before changing any logic, but that's one of the logical changes I'll be making.
| # claim/lock states | ||
| scope :claimed, -> { where.not(locked_at: nil) } | ||
| scope :claimed_by, ->(worker) { where(locked_by: worker.name) } | ||
| scope :unclaimed, -> { where(locked_at: nil) } |
There was a problem hiding this comment.
curious about the motivation behind this rename. i understand using claimed terminology, but should we also consider renaming the columns to claimed_at and claimed_by to align with the scope naming?
There was a problem hiding this comment.
Yeah, I wasn't sure where to have this conversation exactly, so I figured I'd just propose it via a non-impacting name change. I don't mind the column itself being a "lock" because I'm planning on updating these scopes to account for the lock timeout.
So claimed becomes "locked within timeout" and unclaimed becomes "not locked or timed out"
| scope :workable_by, ->(worker) { | ||
| ready_to_run(worker.name) | ||
| pending | ||
| .merge(unclaimed.or(where(arel_table[:locked_at].lt(db_time_now - lock_timeout)))) |
There was a problem hiding this comment.
can we bake the lock timeout into the unclaimed scope? if something has been claimed beyond the lock timeout, it's essentially unclaimed as far as the pickup query is concerned
There was a problem hiding this comment.
Yep, that's one of the logical changes I have queued up, but it will impact the monitor's queries so I want to get the test harness in place first.
2 participants
The goal is to create some unifying structure around the way that scopes are defined and organized in the ActiveRecord
Jobclass, without changing the actual SQL in any meaningful way. (This sets me up to make meaningful SQL changes in future PRs!)In addition to using
Arelforgt/lt/gteq/lteqoperations, I renamed a few concepts:not_failed=>livelocked=>claimednot_locked=>not_claimedI also added two new scopes:
future(where run_at > now)pending(where run_at <= now)Again, this should not meaningfully change any SQL! Only minor formatting changes without changing the meaning of each query. (I'm using the tests to prove this.)
/no-platform