Skip to content

[ON HOLD][13.0] Add job_auto_delay decorator #270

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.

Sign up for GitHub

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

Jump to bottom
Closed
guewen wants to merge 1 commit into from
Closed

[ON HOLD][13.0] Add job_auto_delay decorator #270

guewen wants to merge 1 commit into from

Conversation

@guewen
Copy link
Member

@guewen guewen commented Oct 28, 2020 •
edited
Loading

This is a new decorator, to use instead of @job on methods,
that transforms a method to a method automatically delayed as job when
called.

A typical use case is when a method in a module we don't control is called
synchronously in the middle of another method, and we'd like all the calls
to this method become asynchronous.

Closes #238

lmignon
lmignon approved these changes Oct 29, 2020
View reviewed changes
Copy link
Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

LGTM and good idea! (Code review)

guewen pushed a commit to camptocamp/ddmrp that referenced this pull request Oct 29, 2020
Add ddmrp_cron_actions_as_job
24919eb 
It makes every calls to "cron_actions" run in queue jobs.
The jobs have an identity key with "identity_exact", meaning that only
one occurence of a job for the same buffer with the same arguments
(only_nfp) will be created at a time (e.g. when the state of a
stock.move is changed several times in the same transaction or in
a different transaction in a short timeframe).

It needs OCA/queue#270
@guewen guewen mentioned this pull request Oct 29, 2020
Merged
jgrandguillaume
jgrandguillaume approved these changes Oct 29, 2020
View reviewed changes
Copy link
Member

@jgrandguillaume jgrandguillaume left a comment

Choose a reason for hiding this comment

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

This is so elegant 👍

simahawk
simahawk approved these changes Oct 29, 2020
View reviewed changes
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.

LG, just small glitches.
BTW what about #255 ?

@guewen guewen mentioned this pull request Oct 29, 2020
Closed
@guewen guewen force-pushed the 13.0-job_auto_delay branch from cd49316 to 9659afe Compare October 29, 2020 10:45
@guewen
Copy link
Member Author

guewen commented Oct 29, 2020

@simahawk I fixed the small glitches and force-pushed directly

etobella
etobella approved these changes Oct 29, 2020
View reviewed changes
Copy link
Member

@etobella etobella left a comment

Choose a reason for hiding this comment

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

Great idea @guewen this is something I always needed and had to solve using context keys. This is much more cleaner 😄

@guewen
Copy link
Member Author

guewen commented Oct 29, 2020

Do not merge yet, I'm investigating an error:

AttributeError: method cron_actions on stock.buffer(5577,) is not allowed to be delayed, it should be decorated with odoo.addons.queue_job.job.job

And I suspect it happens when other modules redefine the same method. To be continued...

@guewen guewen changed the title [13.0] Add job_auto_delay decorator [ON HOLD][13.0] Add job_auto_delay decorator Oct 29, 2020
@guewen
Copy link
Member Author

guewen commented Oct 29, 2020

And I suspect it happens when other modules redefine the same method.

I confirm this. I hope I can find a way to prevent this.

@guewen
Copy link
Member Author

guewen commented Oct 29, 2020

AttributeError: method cron_actions on stock.buffer(5577,) is not allowed to be delayed, it should be decorated with odoo.addons.queue_job.job.job

This is not an issue only with @job_auto_delay, we have the error with @job.
If I have such dependencies:

module_a
  |- module_b
  |- module_c

module_b and module_c have no dependencies on each other.

In module_a, we have a method StockMove._update_foo.
In module_b, we add @job on StockMove._update_foo.
In module_c, we have another override of StockMove._update_foo, without decorator.

It seems that depending on the loading order, we'll lose the properties added by @job.

Add job_auto_delay decorator
68d6bc0 
This is a new decorator, to use instead of @job on methods,
that transforms a method to a method automatically delayed as job when
called.

A typical use case is when a method in a module we don't control is called
synchronously in the middle of another method, and we'd like all the calls
to this method become asynchronous.
@guewen guewen force-pushed the 13.0-job_auto_delay branch from 9659afe to 68d6bc0 Compare October 29, 2020 14:10
This was referenced Oct 29, 2020
Closed
Merged
@guewen
Copy link
Member Author

guewen commented Nov 2, 2020

I proposed #275 that works by patching the method instead of using a decorator.
It needs #274 that I still have to finalize.

@guewen guewen closed this Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jgrandguillaume jgrandguillaume jgrandguillaume approved these changes

@simahawk simahawk simahawk approved these changes

@lmignon lmignon lmignon approved these changes

@etobella etobella etobella approved these changes

Assignees

No one assigned

Labels

approved needs fixing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[feature] Add job_auto_delay decorator

6 participants

@guewen @jgrandguillaume @simahawk @lmignon @etobella @OCA-git-bot