Skip to content

[16.0] Add 'Allow Commit' option on job functions - #910

Merged
OCA-git-bot merged 8 commits into
OCA:16.0from
acsone:16.0-add-optional-new-cr
Apr 9, 2026
Merged

[16.0] Add 'Allow Commit' option on job functions#910
OCA-git-bot merged 8 commits into
OCA:16.0from
acsone:16.0-add-optional-new-cr

Conversation

@lmignon

@lmignonlmignon commented Mar 17, 2026

Copy link
Copy Markdown
Contributor

Backport of #899

guewenand others added 8 commits March 17, 2026 16:25
@guewen@lmignon
Add 'Allow Commit' option on job functions
f2bfda9
It is forbidden to commit inside a job, because it releases the job lock
and can cause it to start again, while still being run, by the dead jobs
requeuer. For some use cases, it may actually be legitimate, or at least
be needed in the short term before actual updates in the code.
A new option on the job function, false by default, allow to run the job
in a new transaction, at the cost of an additional connection +
transaction overhead.
Related to OCA#889
@guewen@lmignon
Add parameter to allow commit by default in jobs
feb8a0f
False on new databases, True on existing databases.
Should always be False by default on future versions.
@guewen@lmignon
Revert "Add parameter to allow commit by default in jobs"
2929b6d
This reverts commit b4f3bec.
@guewen@lmignon
Improve documentation on allow commit
649d680
@guewen@lmignon
Fix missing job.env setter
63246bf
As the controller changes env on Job instances.
@guewen@lmignon
Add failure retry in queue job test job
707abfe
@guewen@lmignon
Simplify job env management
65dc65c
@guewen@lmignon
Update upgrade warning link
a6459c4
@OCA-git-bot

OCA-git-bot commented Mar 17, 2026

Copy link
Copy Markdown
Contributor

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

@lmignon
lmignon marked this pull request as ready for review March 18, 2026 08:09
florian-dacosta
florian-dacosta approved these changes Mar 26, 2026
View reviewed changes

@florian-dacostaflorian-dacosta 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.

LGTM (tested)

@florian-dacostaflorian-dacosta mentioned this pull request Mar 26, 2026
Open
@sbidoul

sbidoul commented Mar 26, 2026

Copy link
Copy Markdown
Member

@guewen I kind of remember you could set allow_commit in with_delay() in your original implementation. Was that removed on purpose, or is it me who imagined it?

@guewen

guewen commented Mar 26, 2026

Copy link
Copy Markdown
Member

@sbidoul It was only in your head 😂 I didn't think about that. Do you see a use case for this? For methods that accept a "commit=True" parameter?

@florian-dacosta

florian-dacosta commented Mar 26, 2026

Copy link
Copy Markdown
Contributor

On adapting attachment_queue module, where no queue.job.function was defined, I first tried to pass the allow_commit to the with_delay method here https://github.com/OCA/server-tools/pull/3577/changes#diff-21c8389cfe9291d701d45d4860731ff11ac1ce8ddd2317a07479852488bca7b9R57) I was kind of surprised it was not implemented. But I am not sure it is necessary to change anything about this

@sbidoul

sbidoul commented Mar 26, 2026

Copy link
Copy Markdown
Member

Yes that was what prompted my question ^.

@guewen

guewen commented Mar 26, 2026

Copy link
Copy Markdown
Member

I suppose most of the time, we know ahead of time when a job method will/should commit or not, so it makes more sense on the job function. If you had to set .with_delay(allow_commit=True) in all the delayed calls of the job, it's not the place where the parameter should be IMO.

@sbidoul

sbidoul commented Mar 27, 2026

Copy link
Copy Markdown
Member

@lmignon is this good to go?

florian-dacosta
florian-dacosta reviewed Mar 27, 2026
View reviewed changes
Comment threadqueue_job/views/queue_job_function_views.xml
@guewen

guewen commented Apr 9, 2026

Copy link
Copy Markdown
Member

/ocabot merge major

@OCA-git-bot

OCA-git-bot commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-910-by-guewen-bump-major, awaiting test results.

@OCA-git-bot
OCA-git-bot merged commit 151dc60 into OCA:16.0Apr 9, 2026
6 of 7 checks passed
@OCA-git-bot

OCA-git-bot commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

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

@lmignon
lmignon deleted the 16.0-add-optional-new-cr branch April 9, 2026 10:04
@eantoneseantones mentioned this pull request Apr 28, 2026
Closed
@eantoneseantones mentioned this pull request Apr 28, 2026
Merged
@deeniizdeeniiz mentioned this pull request Apr 29, 2026
Open
@eantoneseantones mentioned this pull request Jul 2, 2026
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guewenguewenguewen approved these changes
+1 more reviewer
@florian-dacostaflorian-dacostaflorian-dacosta approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approvedmerged 🎉

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@lmignon@OCA-git-bot@sbidoul@guewen@florian-dacosta