Skip to content

[19.0][FIX] edi_core_oca: Set exchange record as failed after max retries - #323

Closed
grindtildeath wants to merge 1 commit into
OCA:19.0from
camptocamp:19.0-fix-queue_max_retries
Closed

[19.0][FIX] edi_core_oca: Set exchange record as failed after max retries#323
grindtildeath wants to merge 1 commit into
OCA:19.0from
camptocamp:19.0-fix-queue_max_retries

Conversation

@grindtildeath

Copy link
Copy Markdown
Contributor

No description provided.

@OCA-git-bot

Copy link
Copy Markdown
Contributor

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

@simahawk simahawk 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.

I don't really like this implementation as #321.

First of all we should consider if it really make sense: the state of the job is not the state of the record and we cannot assume that this behavior will be ok for everybody.

Second, we should consider other options rather than trying to guess a state by looking for jobs.

The first ones that come to my mind:

  1. by design the job function handle the state -> you can do this in your own function
  2. catch FailedJobError, check __cause__ attribute to see if it's an instance of RetryableJobError (set by the from directive)
  3. improve job.perform to raise a specific MaxRetryError and catch directly

https://github.com/OCA/queue/blob/18.0/queue_job/job.py#L506

There might be other ways as well that I don't see at the moment 👼

@simahawk

Copy link
Copy Markdown
Contributor

I've made a quick draft for opt 3 OCA/queue#954

@grindtildeath

Copy link
Copy Markdown
Contributor Author

@simahawk the issue with using a specific exception type for max retries reached is that such exception will never reach the code being executed by the queue job.

Hence this is why I went with an "on fail hook" in OCA/queue#955 that is used by #324 to supersede this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants