Skip to content
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.

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

[16.0][IMP] queue_job: run specific hook method after max_retries #674

Open
wants to merge 2 commits into
base: 16.0
Choose a base branch
from

Conversation

QuocDuong1306
Copy link

When a job has been tried for max_retries but still fails, final FailedJobError is raised and job is set to Failed.

This PR enables developers to run a specific hook method when this happens.

When a job has been tried for max_retries but still fails, final FailedJobError is raised and job is set to Failed.
This PR enables developers to run a specific hook method when this happens.
@OCA-git-bot
Copy link
Contributor

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

@@ -527,6 +527,21 @@ def perform(self):
elif not self.max_retries: # infinite retries
raise
elif self.retry >= self.max_retries:
hook = f"{self.method_name}_on_max_retries_reached"
Copy link
Contributor

Choose a reason for hiding this comment

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

I am generally not a fan of interpolating method names. Pass on_exception as an additional argument to delayable/with_delay instead?

Perhaps the scope could be slightly broader as well? Give the developer a chance to handle all types of exception, not just FailedJobError?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • interpolating method names is quite a common pattern in odoo code: see lots of getattr in the codebase :)
  • quite elegant imho to be able to define method_name and method_name_on_max_retries_reached nearby, but of course it's a bit subjective
  • regarding your last point, that's an interesting idea but it feels quite natural to handle exceptions in the job code itself, e.g. in the EDI framework here

Copy link
Contributor

Choose a reason for hiding this comment

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

A more declarative approach could be to use a decorator but it will likely add complexity.
@QuocDuong1306 could you please update the docs?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @simahawk , I updated the docs

Choose a reason for hiding this comment

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

I would say whenever job reaches failed state, it would be useful to have a hook, to do something, not when it just failed after max retries, but failed for any reason?

For example, issue described here: #618

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point. Yet, I think you can subscribe to that particular event easily (job switching to failed).
In fact we could subscribe even in this case and check the max retry counter.
@guewen did you have something in mind regarding handling failures?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants