Deprecate definition of job priority, change to "smaller number is higher priority" to align with Active Job definition #883
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
segiddins
reviewed
Mar 8, 2023
bensheldon
force-pushed
the
deprecate_reverse_priority
branch
from
9add25f
to
76b8fb9
Compare
bensheldon
force-pushed
the
deprecate_reverse_priority
branch
from
76b8fb9
to
f0ab63d
Compare
…gher priority" to align with Active Job definition.
bensheldon
force-pushed
the
deprecate_reverse_priority
branch
from
f0ab63d
to
c35f852
Compare
|
@bensheldon, does this also affect the index on In an existing app (with migrations from prior to this change), I have this index: add_index :good_jobs, [:priority, :created_at], order: { priority: "DESC NULLS LAST", created_at: :asc },Does that need to become Apologies if this is a dupe and this was already on your radar. |
coreyaus
added a commit
to coreyaus/good_job
that referenced
this pull request
Jul 10, 2023
mkrfowler
added a commit
to mkrfowler/good_job
that referenced
this pull request
Jan 14, 2024
The preexisting index (introduced in bensheldon#726) gave a direct answer to the materialized subquery used to find candidate jobs, subject to the ordering rules that were in place at that time. In bensheldon#883, `GoodJob` deprecated that ordering in favour of a lower-is-more-important scheme, aligning with Active Job. When there are a substantial number of completed jobs, the `priority desc` index does not allow for a straight index scan, and may not actually be used by the planner. Aligning the sort orders here allows for the subquery to be satisfied directly.
mkrfowler
added a commit
to mkrfowler/good_job
that referenced
this pull request
Jan 14, 2024
The preexisting index (introduced in bensheldon#726) gave a direct answer to the materialized subquery used to find candidate jobs, subject to the ordering rules that were in place at that time. In bensheldon#883, `GoodJob` deprecated that ordering in favour of a lower-is-more-important scheme, aligning with Active Job. When there are a substantial number of completed jobs, the `priority desc` index does not allow for a straight index scan, and may not actually be used by the planner. Aligning the sort orders here allows for the subquery to be satisfied directly. Although `ASC NULLS LAST` is the default (as observable in the generated schema) it seems appropriate to be explicit in the actual index declaration, although mostly for consistency with the previous version.
mkrfowler
added a commit
to mkrfowler/good_job
that referenced
this pull request
Jan 14, 2024
The preexisting index (introduced in bensheldon#726) gave a direct answer to the materialized subquery used to find candidate jobs, subject to the ordering rules that were in place at that time. In bensheldon#883, `GoodJob` deprecated that ordering in favour of a lower-is-more-important scheme, aligning with Active Job. When there are a substantial number of completed jobs, the `priority desc` index does not allow for a straight index scan, and may not actually be used by the planner. Aligning the sort orders here allows for the subquery to be satisfied directly. Although `ASC NULLS LAST` is the default (as observable in the generated schema) it seems appropriate to be explicit in the actual index declaration, although mostly for consistency with the previous version.
mkrfowler
added a commit
to mkrfowler/good_job
that referenced
this pull request
Jan 14, 2024
The preexisting index (introduced in bensheldon#726) gave a direct answer to the materialized subquery used to find candidate jobs, subject to the ordering rules that were in place at that time. In bensheldon#883, `GoodJob` deprecated that ordering in favour of a lower-is-more-important scheme, aligning with Active Job. When there are a substantial number of completed jobs, the `priority desc` index does not allow for a straight index scan, and may not actually be used by the planner. Aligning the sort orders here allows for the subquery to be satisfied directly. Although `ASC NULLS LAST` is the default (as observable in the generated schema) it seems appropriate to be explicit in the actual index declaration, although mostly for consistency with the previous version.
mkrfowler
added a commit
to mkrfowler/good_job
that referenced
this pull request
Jan 14, 2024
The preexisting index (introduced in bensheldon#726) gave a direct answer to the materialized subquery used to find candidate jobs, subject to the ordering rules that were in place at that time. In bensheldon#883, `GoodJob` deprecated that ordering in favour of a lower-is-more-important scheme, aligning with Active Job. When there are a substantial number of completed jobs, the `priority desc` index does not allow for a straight index scan, and may not actually be used by the planner. Aligning the sort orders here allows for the subquery to be satisfied directly. Although `ASC NULLS LAST` is the default (as observable in the generated schema) it seems appropriate to be explicit in the actual index declaration, although mostly for consistency with the previous version.
mkrfowler
added a commit
to mkrfowler/good_job
that referenced
this pull request
Jan 14, 2024
The preexisting index (introduced in bensheldon#726) gave a direct answer to the materialized subquery used to find candidate jobs, subject to the ordering rules that were in place at that time. In bensheldon#883, `GoodJob` deprecated that ordering in favour of a lower-is-more-important scheme, aligning with Active Job. When there are a substantial number of completed jobs, the `priority desc` index does not allow for a straight index scan, and may not actually be used by the planner. Aligning the sort orders here allows for the subquery to be satisfied directly. Although `ASC NULLS LAST` is the default (as observable in the generated schema) it seems appropriate to be explicit in the actual index declaration, although mostly for consistency with the previous version.
bensheldon
pushed a commit
that referenced
this pull request
Jan 19, 2024
…1213) The preexisting index (introduced in #726) gave a direct answer to the materialized subquery used to find candidate jobs, subject to the ordering rules that were in place at that time. In #883, `GoodJob` deprecated that ordering in favour of a lower-is-more-important scheme, aligning with Active Job. When there are a substantial number of completed jobs, the `priority desc` index does not allow for a straight index scan, and may not actually be used by the planner. Aligning the sort orders here allows for the subquery to be satisfied directly. Although `ASC NULLS LAST` is the default (as observable in the generated schema) it seems appropriate to be explicit in the actual index declaration, although mostly for consistency with the previous version.
francois-ferrandis
added a commit
to betagouv/rdv-service-public
that referenced
this pull request
Mar 4, 2024
francois-ferrandis
added a commit
to betagouv/rdv-service-public
that referenced
this pull request
Mar 4, 2024
* Update gem * Generate and run GoodJob migrations * Switch to Active Job's priority convention See bensheldon/good_job#883 * FIx spec on priority * Ignore GoodJob migrations in Rubocop * Re-add safety_assured that was wiped when regenerating migrations
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reported in #524 (comment).
Active Job describes priority as "lower is more priority".