Skip to content

[18.0][PORT] queue_job: remove dead jobs requeuer cron and automatically requeue dead jobs #749

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 7 commits into from
Sep 17, 2025
Merged

[18.0][PORT] queue_job: remove dead jobs requeuer cron and automatically requeue dead jobs #749

OCA-git-bot merged 7 commits into from
Sep 17, 2025

Conversation

@sbidoul
Copy link
Member

@sbidoul sbidoul commented Mar 3, 2025 •
edited by simahawk
Loading

FW port of #748 + #816

@OCA-git-bot
Copy link
Contributor

OCA-git-bot commented Mar 3, 2025

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

@sbidoul sbidoul force-pushed the 18.0-748-port branch 2 times, most recently from 11688d3 to 58bf17f Compare March 3, 2025 17:49
@hbrunn
Copy link
Member

hbrunn commented Mar 21, 2025

@sbidoul looks like you have to rewrite this to use savepoints?

@sbidoul
Copy link
Member Author

sbidoul commented Mar 21, 2025

I'm note sure it's possible to test that with savepoints, since we test database locks.

@AnizR

sbidoul
sbidoul commented Mar 21, 2025
View reviewed changes
queue_job/tests/test_requeue_dead_job.py Outdated
self.assertEqual(1, len(locks))

# commit to update queue_job records in DB
self.env.cr.commit() # pylint: disable=E8102
Copy link
Member Author

@sbidoul sbidoul Mar 21, 2025

Choose a reason for hiding this comment

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

@AnizR in this test, is it necessary to commit?

Copy link
Contributor

@AnizR AnizR Mar 21, 2025

Choose a reason for hiding this comment

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

Yes, otherwise, the row representing a 'lock' will not be created within the db

sbidoul
sbidoul commented Mar 21, 2025
View reviewed changes
queue_job/tests/test_requeue_dead_job.py Outdated
job_obj.set_started()

job_obj.store()
self.env.cr.commit() # pylint: disable=E8102
Copy link
Member Author

@sbidoul sbidoul Mar 21, 2025

Choose a reason for hiding this comment

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

Here we could maybe create a job with demo data, and run this test post install.

Copy link
Contributor

@AnizR AnizR Mar 21, 2025

Choose a reason for hiding this comment

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

Using data to create some records within the db might be a way to avoid to commit within tests, I'll think about it 👍

@sbidoul sbidoul mentioned this pull request Apr 15, 2025
Merged
baguenth
baguenth reviewed Apr 15, 2025
View reviewed changes
queue_job/migrations/18.0.1.3.0/pre-migration.py Outdated
WHERE
module='queue_job'
AND model='ir.cron'
AND name='ir_cron_queue_job_garbage_collector'
Copy link
Member

@baguenth baguenth Apr 15, 2025

Choose a reason for hiding this comment

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

You may directly want to consider this fix: 46b0d8f

@AnizR AnizR force-pushed the 18.0-748-port branch 2 times, most recently from 42a1df0 to f64b966 Compare June 2, 2025 15:08
@rousseldenis rousseldenis added this to the 18.0 milestone Jun 2, 2025
@AnizR
Copy link
Contributor

AnizR commented Jun 2, 2025

@hbrunn, I came up with the solution of loading test queue.job records within demo.

What do you think about it?

@AnizR AnizR force-pushed the 18.0-748-port branch 2 times, most recently from 5fd02bb to dca717b Compare June 3, 2025 14:11
sbidoul
sbidoul commented Jun 8, 2025
View reviewed changes
queue_job/demo/queue_job_test_lock.xml Outdated
model="queue.job"
eval="('test_enqueued_job',)"
/>
</odoo>
Copy link
Member Author

@sbidoul sbidoul Jun 8, 2025

Choose a reason for hiding this comment

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

@AnizR this file should be removed as it was moved to test_queue_job?

Copy link
Contributor

@AnizR AnizR Jun 23, 2025

Choose a reason for hiding this comment

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

Yes, well spotted!

@AnizR AnizR force-pushed the 18.0-748-port branch 3 times, most recently from 57d2771 to d071197 Compare June 23, 2025 15:38
@sbidoul sbidoul mentioned this pull request Aug 20, 2025
Merged
simahawk
simahawk approved these changes Aug 20, 2025
View reviewed changes
Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

LGTM. Remember to merge w/ nobump.

@sbidoul sbidoul marked this pull request as draft August 20, 2025 09:04
@sbidoul
Copy link
Member Author

sbidoul commented Aug 20, 2025

Setting as draft. There is a conflicts and the tests need some rework.

AnizR and others added 5 commits September 15, 2025 10:11
@AnizR
[IMP] queue_job: remove cron garbage collector and automatically requ…
f10020b 
…eue jobs in timeout

[IMP] queue_job: increment 'retry' when re-queuing job that have been killed
@sbidoul @AnizR
[IMP] queue_job: use queue_job_lock model
4e517f7 
A model is better than a manually managed table as it will
protect the table from deletion by database_cleanup.
@sbidoul @AnizR
[IMP] queue_job: tweak comment and warning message
5aaf399
@baguenth @AnizR
[FIX] queue_job: Adapted migration to properly delete the invalid cro…
10395fd 
…n job
@AnizR
[IMP] queue_job: remove DB commits within test of requeue
8a346da
@AnizR
Copy link
Contributor

AnizR commented Sep 15, 2025

I reworked the tests and decided to isolate the tests in test_queue_job.
Moreover, I managed to remove the .commit() calls.

FrancoMaxime
FrancoMaxime reviewed Sep 15, 2025
View reviewed changes
Copy link
Member

@FrancoMaxime FrancoMaxime left a comment

Choose a reason for hiding this comment

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

LGTM: code review

@OCA-git-bot
Copy link
Contributor

OCA-git-bot commented Sep 15, 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 sbidoul marked this pull request as ready for review September 17, 2025 14:57
@sbidoul
Copy link
Member Author

sbidoul commented Sep 17, 2025

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

OCA-git-bot commented Sep 17, 2025

On my way to merge this fine PR!
Prepared branch 18.0-ocabot-merge-pr-749-by-sbidoul-bump-major, awaiting test results.

@OCA-git-bot
Copy link
Contributor

OCA-git-bot commented Sep 17, 2025

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

@OCA-git-bot OCA-git-bot merged commit b4c689d into OCA:18.0 Sep 17, 2025
7 checks passed
@sbidoul sbidoul deleted the 18.0-748-port branch September 17, 2025 15:21
@simahawk simahawk mentioned this pull request Nov 20, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@simahawk simahawk simahawk approved these changes

+4 more reviewers

@FrancoMaxime FrancoMaxime FrancoMaxime left review comments

@AnizR AnizR AnizR left review comments

@baguenth baguenth baguenth left review comments

@lmarion-source lmarion-source lmarion-source approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@AnizR AnizR

Labels

approved merged 🎉 needs review ready to merge

Projects

None yet

Milestone

18.0

Development

Successfully merging this pull request may close these issues.

10 participants

@sbidoul @OCA-git-bot @hbrunn @AnizR @simahawk @FrancoMaxime @lmarion-source @baguenth @rousseldenis @zamberjo