Skip to content

[17.0][PORT] queue_job: remove dead jobs requeuer cron and automatically requeue dead jobs #748

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.

Sign up for GitHub

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

Jump to bottom
Merged
OCA-git-bot merged 3 commits into from
Mar 17, 2025
Merged

[17.0][PORT] queue_job: remove dead jobs requeuer cron and automatically requeue dead jobs #748

OCA-git-bot merged 3 commits into from
Mar 17, 2025

Conversation

@sbidoul
Copy link
Member

@sbidoul sbidoul commented Feb 28, 2025

FW port of #716

AnizR and others added 2 commits February 28, 2025 08:54
@AnizR @sbidoul
[IMP] queue_job: remove cron garbage collector and automatically requ…
5d12354 
…eue jobs in timeout

[IMP] queue_job: increment 'retry' when re-queuing job that have been killed
@sbidoul
[IMP] queue_job: use queue_job_lock model
274c7b3 
A model is better than a manually managed table as it will
protect the table from deletion by database_cleanup.
@OCA-git-bot
Copy link
Contributor

OCA-git-bot commented Feb 28, 2025

Hi @guewen,
some modules you are maintaining are being modified, check this out!

@sbidoul sbidoul requested a review from AnizR February 28, 2025 07:56
@sbidoul sbidoul mentioned this pull request Feb 28, 2025
Merged
AnizR
AnizR suggested changes Feb 28, 2025
View reviewed changes
Copy link
Contributor

@AnizR AnizR left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the port forward!

No remarks to do regarding the logic, it still works: queue_job isn't different between v16 and v17.

Small question: Shouldn't we need to change the module version since we are adding a migration script?

@sbidoul
Copy link
Member Author

sbidoul commented Feb 28, 2025

Shouldn't we need to change the module version since we are adding a migration script?

I was thinking to merge with a minor bump, but changing it explicitly is safer. Done.

AnizR
AnizR approved these changes Feb 28, 2025
View reviewed changes
Copy link
Contributor

@AnizR AnizR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, nice. Everything looks good to me now 👍

@sbidoul sbidoul mentioned this pull request Mar 3, 2025
Merged
@OCA-git-bot
Copy link
Contributor

OCA-git-bot commented Mar 5, 2025

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@sbidoul
Copy link
Member Author

sbidoul commented Mar 17, 2025

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

OCA-git-bot commented Mar 17, 2025

What a great day to merge this nice PR. Let's do it!
Prepared branch 17.0-ocabot-merge-pr-748-by-sbidoul-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 7b8906e into OCA:17.0 Mar 17, 2025
7 checks passed
@OCA-git-bot
Copy link
Contributor

OCA-git-bot commented Mar 17, 2025

Congratulations, your PR was merged at f327ed0. Thanks a lot for contributing to OCA. ❤️

@sbidoul sbidoul deleted the 17.0-716-port branch March 17, 2025 13:09
@sbidoul sbidoul mentioned this pull request Mar 17, 2025
Merged
sersanchus
sersanchus reviewed Aug 14, 2025
View reviewed changes
queue_job/jobrunner/runner.py
state=(
CASE
WHEN
max_retries IS NOT NULL AND
Copy link

@sersanchus sersanchus Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe there's a problem here? max_retries can be 0 and is well documented that it has to retry forever...

Copy link
Member Author

@sbidoul sbidoul Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly, indeed. cc/ @AnizR

Copy link
Member

@zamberjo zamberjo Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a possible solution in #816, mind taking a look?

@sbidoul @AnizR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guewen guewen guewen approved these changes

+4 more reviewers

@sersanchus sersanchus sersanchus left review comments

@zamberjo zamberjo zamberjo left review comments

@hparfr hparfr hparfr approved these changes

@AnizR AnizR AnizR approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approved merged 🎉 ready to merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@sbidoul @OCA-git-bot @guewen @sersanchus @zamberjo @hparfr @AnizR