[18.0] Add 'Allow Commit' option on job functions#899
[18.0] Add 'Allow Commit' option on job functions#899
Conversation
|
Hi @sbidoul, |
guewen
force-pushed
the
18.0-add-optional-new-cr
branch
2 times, most recently
from
75725a3 to
8f24e19
Compare
| "enable, func_name, kwargs.\n" | ||
| "See the module description for details.", | ||
| ) | ||
| run_in_new_cursor = fields.Boolean( |
There was a problem hiding this comment.
Would it make sense to name this option with something that conveys "allow commit in job". Not sure.
There was a problem hiding this comment.
I'll do that, just allow_commit should be enough?
| related_action_func_name=None, | ||
| related_action_kwargs={}, | ||
| job_function_id=None, | ||
| run_in_new_cursor=False, |
There was a problem hiding this comment.
In the interest of magically un-breaking other modules that depend on queue_job, perhaps run_in_new_cursor could default to True for 18.0 and then back to False for 19.0?
There was a problem hiding this comment.
Or get the default value from a system parameter (False by default) ?
There was a problem hiding this comment.
System parameter that is False by default but with a migration that sets it to True automatically on existing databases?
We could modify _prevent_commit to update the job function option when the error happens, that would give the best of both worlds, but might be a bit too much; also I fear it could create other problems with several jobs attempting to update the same job function.
There was a problem hiding this comment.
System parameter that is False by default but with a migration that sets it to True automatically on existing databases?
Why not. I'd be fine with breaking by default with a message explaining all the solutions and tradeoff, in order to raise awareness, but if people prefer to not break by default, good for me too.
We could modify
_prevent_committo update the job function option when the error happens, that would give the best of both worlds, but might be a bit too much; also I fear it could create other problems with several jobs attempting to update the same job function.
Sounds overkill to me too.
There was a problem hiding this comment.
I have mixed feelings about the 2 options. My preference would go about breaking by default with an explanation, because otherwise nobody will care and databases will have the option enabled on all jobs forever. On the other side, I'm sure that in many situations nobody monitor the jobs and the issue will be discovered after some time, maybe after real issues due to non-running jobs (well there is a point about accountability regarding monitoring errors and jobs here).
I updated the PR yesterday evening with a system parameter and migration to allow commits by default, but I tend to favor breaking by default after a night sleep, so I'm tempted to remove it. We could add a link in the error message to a page such as this draft: https://github.com/OCA/queue/wiki/%5BDRAFT%5D-Upgrade-warning:-commits-inside-jobs
There was a problem hiding this comment.
My preference would go about breaking by default with an explanation, because otherwise nobody will care and databases will have the option enabled on all jobs forever. On the other side, I'm sure that in many situations nobody monitor the jobs and the issue will be discovered after some time, maybe after real issues due to non-running jobs (well there is a point about accountability regarding monitoring errors and jobs here).
Yeah, doubling the number of database cursors used for all jobs is not the best. 😓 I also lean towards breaking by default and removing the config parameter. Make sure to semver major!
There was a problem hiding this comment.
I updated with a link to the explanation
guewen
force-pushed
the
18.0-add-optional-new-cr
branch
2 times, most recently
from
018331b to
7bc3c77
Compare
guewen
changed the title
guewen
force-pushed
the
18.0-add-optional-new-cr
branch
from
7bc3c77 to
2dec167
Compare
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
False on new databases, True on existing databases. Should always be False by default on future versions.
guewen
force-pushed
the
18.0-add-optional-new-cr
branch
from
2dec167 to
b4f3bec
Compare
4 participants
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 #889