Skip to content

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

Merged
OCA-git-bot merged 3 commits into
OCA:17.0from
acsone:17.0-716-port
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
OCA:17.0from
acsone:17.0-716-port

Conversation

@sbidoul

@sbidoul sbidoul commented Feb 28, 2025

Copy link
Copy Markdown
Member

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

OCA-git-bot commented Feb 28, 2025

Copy link
Copy Markdown
Contributor

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

@AnizR AnizR left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

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

sbidoul commented Feb 28, 2025

Copy link
Copy Markdown
Member Author

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

@AnizR AnizR left a comment

Copy link
Copy Markdown
Contributor

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

OCA-git-bot commented Mar 5, 2025

Copy link
Copy Markdown
Contributor

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

sbidoul commented Mar 17, 2025

Copy link
Copy Markdown
Member Author

/ocabot merge nobump

@OCA-git-bot

OCA-git-bot commented Mar 17, 2025

Copy link
Copy Markdown
Contributor

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
@OCA-git-bot

OCA-git-bot commented Mar 17, 2025

Copy link
Copy Markdown
Contributor

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
Comment thread queue_job/jobrunner/runner.py
state=(
CASE
WHEN
max_retries IS NOT NULL AND

@sersanchus sersanchus Aug 14, 2025

Copy link
Copy Markdown

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...

@sbidoul sbidoul Aug 14, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Possibly, indeed. cc/ @AnizR

@zamberjo zamberjo Aug 19, 2025

Copy link
Copy Markdown
Member

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