Repository navigation
[FIX] queue_job: apply max_retries to PostgreSQL concurrency errors - #1009
richard-willdooit wants to merge 2 commits into
Conversation
|
All commit authors signed the Contributor License Agreement. |
OCA-git-bot
added
series:19.0
mod:queue_job
The typical transaction serialization errors are retried, but the conversion from OperationalError to RetryableJobError happens in the runners, outside Job.perform() - and perform() is the only place max_retries is consulted. retry_postpone() simply writes a new eta and requeues, so a job that keeps colliding is retried for ever while its retry counter climbs past the limit and it never reaches the failed state. Seen in production on a long report-building job: it sat at try 10 of a maximum of 5, each attempt running for some eight minutes, until it was closed by hand. Convert the error inside Job.perform() instead, so it falls into the RetryableJobError handling that is already there and is subject to max_retries like any other retryable error. The duplicated handling in queue_job's controller and in queue_job_cron_jobrunner then comes out, and PG_RETRY moves beside DEFAULT_MAX_RETRIES. _enqueue_dependent_jobs keeps its own OperationalError handling: that is a separate retry loop around the dependency update, not the job itself. Tests: a serialization failure is retried and gives up at max_retries, and any other OperationalError is not retried at all.
e1f5ea6 to
b0b8956
Compare
|
|
||
| _logger = logging.getLogger(__name__) | ||
|
|
||
| PG_RETRY = 5 # seconds |
There was a problem hiding this comment.
Maybe worth importing it from where it stands now for backward compat.
Not a blocker from my POV.
There was a problem hiding this comment.
Yeah maybe worth in case someone customized the default of 5 seconds.
There was a problem hiding this comment.
Happy to add the re-export, so imports from controllers.main keep working. But it only keeps the import working. If someone changed the value at the old location, job.py won't see it - the delay would quietly go back to 5 seconds.
I'd rather offer a supported way to do it, which ties into your other comment: use the job's retry_pattern when it has one, and fall back to PG_RETRY otherwise.
|
@guewen ping |
|
This PR has the |
| # limit, so it retries for ever. | ||
| if err.pgcode not in PG_CONCURRENCY_ERRORS_TO_RETRY: | ||
| raise | ||
| raise RetryableJobError(err.pgerror, seconds=PG_RETRY) from err |
There was a problem hiding this comment.
I wonder if we should keep this forced PG_RETRY or just use the same behavior as other retries.
Originally, the serialization error were not checking the max retries on purpose, because I considered they had to succeed at some point. But then, 5 seconds of wait can be long and there is no override mechanism for this, but if we do remove seconds=PG_RETRY, we get the default retry which is also 5 seconds, but allow to be customized in the job function.
There was a problem hiding this comment.
Hi @guewen
Removing it wouldn't give 5 seconds: with no retry_pattern on the job function, _get_retry_seconds falls back to RETRY_INTERVAL, which is 10 minutes, so a serialization conflict that clears almost instantly would wait 10 minutes. Jobs that do define a pattern usually tune it for slower failures.
I'd suggest keeping PG_RETRY as the default and letting a job's retry_pattern override it when one is set (seconds=None if self.job_config.retry_pattern else PG_RETRY). That keeps current behaviour for everyone else and gives the override mechanism you mention.
Happy to push that if it works for you.
There was a problem hiding this comment.
You are right, I misread. I like the idea to use the retry pattern if set or fallback on the constant
There was a problem hiding this comment.
You are right, I misread. I like the idea to use the retry pattern if set or fallback on the constant
…y delay Follow-up to the review on OCA#1009. A job that keeps hitting serialization failures was waiting 5 seconds before each retry, ignoring its specific retry_pattern. Now, we pass no delay at all when the job function declares a retry_pattern, and the pattern will apply as it does for any other RetryableJobError. Jobs without a pattern are still passed PG_RETRY and still wait 5 seconds. The constant cannot simply be dropped for them: _get_retry_seconds() would then fall back to RETRY_INTERVAL - ten minutes - for a conflict that usually clears at once. A job function that declares a retry_pattern will change behaviour: a serialization failure now waits what the pattern says. That is the point of it; the job has said how it wants to be retried. The surprise is a pattern that does not cover the early tries - {3: 180} waits RETRY_INTERVAL on tries 1 and 2, so ten minutes where it used to be five seconds. PG_RETRY is also re-exported from queue_job.controllers.main, so that imports from its old home keep working. queue_job_cron_jobrunner had a hardcoded seconds=5, discarding the delay the error asks for, so a retry_pattern would be ignored under that runner; it now passes err.seconds like the controller does. Tests: with no retry_pattern a serialization failure still waits PG_RETRY, and with one it waits what the pattern says.
The problem
The typical PostgreSQL transaction serialization errors are retried, but the conversion from
OperationalErrortoRetryableJobErrorhappens in the runners, outsideJob.perform()— andperform()is the only placemax_retriesis consulted.queue_job/controllers/main.pywraps_try_perform_joband raisesRetryableJobErrorfrom there; the surroundingexcept RetryableJobErrorcallsretry_postpone(), which writes a new eta and requeues.queue_job_cron_jobrunner/models/queue_job.pydoes the same inline, callingpostpone()+set_pending(reset_retry=False)directly.Neither consults
max_retries.perform()has already incrementedretryby the time the error surfaces, so a job that keeps colliding is retried for ever while its counter climbs past the limit, and it never reachesfailed.Seen in production on a long report-building job: it sat at try 10 of a maximum of 5, each attempt running for about eight minutes, until it was closed by hand.
The change
Convert the error inside
Job.perform()instead, so it falls into theRetryableJobErrorhandling already there and is subject tomax_retrieslike any other retryable error:The duplicated handling then comes out of both runners, and
PG_RETRYmoves besideDEFAULT_MAX_RETRIESinjob.py. Net effect is a deletion: one conversion in one place, and both runners behave identically._enqueue_dependent_jobskeeps its ownOperationalErrorhandling — that is a separate retry loop around the dependency update, not around the job.Behaviour
max_retriesmax_retriesFailedJobError, job failsOperationalErrormax_retries = 0Tests
test_queue_jobgains araise_pg_concurrencyoption ontesting_method, backed by a smallOperationalErrorsubclass — psycopg2 exposespgcoderead-only anderrors.lookup()leaves it unset, so the code has to be carried that way. Two cases intest_job.py, mirroring the existingtest_retryable_error:max_retriesOperationalErroris not retriedFull suite on 19.0: 186 tests, 0 failed, 0 errors (
queue_job,test_queue_job,queue_job_cron_jobrunner).