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

[12.0][IMP] edi_oca: don't consider exchange records if jobs have been created #996

Open
wants to merge 1 commit into
base: 12.0
Choose a base branch
from

Conversation

MiquelRForgeFlow
Copy link
Contributor

If two _check_output_exchange_sync are called consecutively, one after the other, maybe due to _execute_next_action method, the following error happens:
Selection_3459
Both _check_output_exchange_sync calls detect the same exchange records, and both create same jobs. The first batch of jobs will be processed but the second ones will fail due to check of state = "new".

@OCA-git-bot
Copy link
Contributor

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

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 12.0-fix-edi-too-much-sync branch 4 times, most recently from a2b52fc to e41329a Compare June 14, 2024 13:51
@MiquelRForgeFlow MiquelRForgeFlow changed the title [IMP] edi_oca: don't consider exchange records if jobs have been created [12.0][IMP] edi_oca: don't consider exchange records if jobs have been created Jun 14, 2024
@MiquelRForgeFlow
Copy link
Contributor Author

@simahawk what's your opinion on this?

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 12.0-fix-edi-too-much-sync branch from e41329a to 7c00765 Compare July 19, 2024 09:24
Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

We should avoid having this kind of relations w/ jobs.
Please see OCA/queue#573

As for the solution on how to prevent the issue to happen... IMO we should rely on
the id key https://github.com/OCA/edi-framework/blob/16.0/edi_oca/models/edi_exchange_record.py#L591

And we should leverage the job chaining for inbound records too to speed up processing.

FYI there's a way to find jobs for a record OCA/edi-framework#93

Regarding the error itself: an alternative would be to simply let the job fail gracefully (meaning set it as done).

BTW I was thinking that we probably need a specific state for when a record is waiting for a job... not sure tho.

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 12.0-fix-edi-too-much-sync branch 2 times, most recently from 4f2effb to 181dffa Compare July 26, 2024 12:06
@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 12.0-fix-edi-too-much-sync branch 2 times, most recently from fd3a90e to 9d6b406 Compare July 26, 2024 12:10
@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 12.0-fix-edi-too-much-sync branch from 9d6b406 to a7f0b8c Compare July 26, 2024 12:11
@MiquelRForgeFlow
Copy link
Contributor Author

@simahawk is it acceptable now? It became simpler.

@MiquelRForgeFlow
Copy link
Contributor Author

MiquelRForgeFlow commented Jul 29, 2024

@simahawk

Regarding the error itself: an alternative would be to simply let the job fail gracefully (meaning set it as done).

Yeah, but silencing possible errors it's bad coding. I prefer avoiding from happening the specific detected ones, and letting the open door to detect other errors.

BTW I was thinking that we probably need a specific state for when a record is waiting for a job... not sure tho.

A new state between "new" and the others, for example "queued" would be nice, but I am afraid that it's complicated, as we should review all the flows and adapt code. I prefer the boolean solution, it's simpler.

@MiquelRForgeFlow
Copy link
Contributor Author

@simahawk could you take a look?

Copy link

github-actions bot commented Jan 5, 2025

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jan 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants