[19.0][IMP] queue_job: Implement on fail hook - #955
Conversation
Allows to execute a model function when the job fails and will not be retried.
|
Just for the record, there were 2 other attempts to manage this
Your approach seem cleaner tho. |
| vals = cls._get_failure_values(job, traceback_txt, orig_exception) | ||
| job.set_failed(**vals) | ||
| job.store() | ||
| job.on_fail_hook(vals) |
There was a problem hiding this comment.
shall we pass the exception obj too?
There was a problem hiding this comment.
Is there any added value to pass the raw exception instead of what's been extracted from it in _get_failure_values ❓
There was a problem hiding this comment.
yes, you can inspect the exception for additional info. Not a must have tho.
| if commit_within_job: | ||
| self.env.cr.commit() # pylint: disable=invalid-commit | ||
|
|
||
| def _test_on_fail_hook(self, **kw): |
There was a problem hiding this comment.
If only for testing,this is not needed
There was a problem hiding this comment.
I get AttributeError: <class 'odoo.addons.queue_job.models.queue_job.QueueJob'> does not have the attribute '_test_on_fail_hook' if I remove it
florentx
left a comment
florentx
left a comment
There was a problem hiding this comment.
CI fails on test_queue_job add-on.
|
IMO the test failure is correct because it runs in a temporary env that is not rolled back. |
|
|
||
| model_name = fields.Char(string="Model", readonly=True) | ||
| method_name = fields.Char(readonly=True) | ||
| on_fail_method_name = fields.Char(readonly=True) |
There was a problem hiding this comment.
I would appreciate consistency in naming: on_fail_hook + on_fail_method + on_fail_method_name is going to make some greps miss.
There was a problem hiding this comment.
Hello @amh-mw
What would you suggest?
As I take naming quite seriously, here was my rationale:
- on_fail_method is a reference to the method (eg when call
with_delay) - on_fail_method_name is the name of the method (which must be on the same model than the queue job)
- on_fail_hook is the internal function of the job that will be calling the on_fail_method
There was a problem hiding this comment.
I might rename all of them (plus on_fail_func that I just noticed) to on_fail. They're on different classes or are method arguments, so I am 85% confident they won't collide.
In honor of this pull request, I'm going to rename _on_done in #937 to check_done to reserve the on_ prefix for hooks.
There was a problem hiding this comment.
on_fail instead of on_fail_hook make sense. For the rest, is true that func is the term that is already present. But not a blocker from my POV.
| description=None, | ||
| channel=None, | ||
| identity_key=None, | ||
| on_fail_method=None, |
There was a problem hiding this comment.
Does it make sense to set this on a per-call basis? I think a "job function" will always have the same "on fail method", and would then configure it on queue.job.function, it simplifies the thinking model (always the same behavior) and it simplifies the code (need to be added only in job_config (no propagation in all calls, no new field on queue.job, ...)
6 participants
Allows to execute a model function when the job fails and will not be retried.