Eager retry for short retry delays #105
Merged
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
brandur
force-pushed
the
brandur-eager-job-retry
branch
from
9b0559c to
91ebc57
Compare
bgentry
approved these changes
Dec 11, 2023
| @@ -72,6 +72,7 @@ WITH locked_jobs AS ( | |||
| WHERE | |||
| state = 'available'::river_job_state | |||
| AND queue = @queue::text | |||
| AND scheduled_at <= now() | |||
There was a problem hiding this comment.
This should be totally fine from a query performance perspective because it's part of the compound fetching index and is already being used for sorting.
There was a problem hiding this comment.
Great. Yeah I figured that the extra predicate is on average going to have very little work to do since the vast majority of rows that aren't ready yet will still be scheduled.
An aspect of River that could currently be considered a bug is that retry delays below five seconds are effectively ignored. This is because when a job errors it always transitions to `retryable` state, and needs to wait for the scheduler to run to transition it back to `available`, and the scheduler run interval is five seconds. This is in practice a problem because it means that the first retry in a retry policy isn't actually one second as claimed in docs, etc., but actually ~5 seconds, and it won't be obvious why. Here, implement an "eager" retry system such that if we detect that the retry delay is smaller than the scheduler's run interval, we place the job immediately into an `available` state, allowing it to be worked sooner. To facilitate this we add a predicate on the "get available" query that checks for `scheduled_at <= now()` so that errored jobs with short retries can be `available` for a short time without being worked immediately. Most of the time this should have no effect because this really only applies to the first retry in any failure (the second is at 16 seconds, which is well above the scheduler's run interval).
brandur
force-pushed
the
brandur-eager-job-retry
branch
from
91ebc57 to
8d39a9e
Compare
|
Excellent. Thanks! Added a changelog and pulling this in. |
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.
An aspect of River that could currently be considered a bug is that
retry delays below five seconds are effectively ignored. This is because
when a job errors it always transitions to
retryablestate, and needsto wait for the scheduler to run to transition it back to
available,and the scheduler run interval is five seconds.
This is in practice a problem because it means that the first retry in a
retry policy isn't actually one second as claimed in docs, etc., but
actually ~5 seconds, and it won't be obvious why.
Here, implement an "eager" retry system such that if we detect that the
retry delay is smaller than the scheduler's run interval, we place the
job immediately into an
availablestate, allowing it to be workedsooner.
To facilitate this we add a predicate on the "get available" query that
checks for
scheduled_at <= now()so that errored jobs with shortretries can be
availablefor a short time without being workedimmediately. Most of the time this should have no effect because this
really only applies to the first retry in any failure (the second is at
16 seconds, which is well above the scheduler's run interval).