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

[WIP] Rework jobs queue #212

Closed
wants to merge 22 commits into from
Closed

[WIP] Rework jobs queue #212

wants to merge 22 commits into from

Conversation

guewen
Copy link
Member

@guewen guewen commented Oct 4, 2016

  • Replace ConnectorSession by odoo.api.Environment (class is still there, we'll keep it a moment if we decide to add backward-compatibility layers)
  • Merge JobStorage and Job classes

Still to do:

  • [queue_job]: allow to delay Model methods
  • [queue_job]: use json instead of pickle for the job arguments (recordsets should be serialized as ['model.name', [ids]])
  • [queue_job]: return job record instead of uuid on delay
  • [queue_job]: make related actions methods of the queue.job model

Depends on #211 (only commits after 3599912 are relevant).

@guewen guewen mentioned this pull request Oct 4, 2016
The reason of being for the separation was to be able to support another
storage than postgres if needed.  Nothing has ever been done in this
way and the current backend storage (postgres) has proved to be
efficient.
@guewen guewen force-pushed the 10.0-rework-jobs branch 3 times, most recently from 670eea6 to 3a5b253 Compare October 4, 2016 14:43
@guewen
Copy link
Member Author

guewen commented Oct 6, 2016

I have some design issues for the jobs on Model methods.

Currently this is how we delay a function:

@job
def my_func(env,recordset, a, k=None):
    pass

my_func.delay(env, recordset, 1, k=1)  # asynchronous
my_func(env, recordset, 1, k=1)  # synchronous
# and when we have additional parameters for the job handling they are added to
# the kwargs (so there are *reserved* kwargs):
my_func.delay(env, recordset, 1, k=1, priority=1, description='test')

On Model methods, according to this definition:

class MyModel(models.Model):
    _name = 'my.model'

    @api.multi
    @job
    def my_method(self, a, k=1):
        pass

We would have:

self.env['my.model'].my_method.delay(1, k=1)  # might be ok
recordset = self.env['my.model'].browse(1)
recordset.my_method.delay(1, k=1) # not ok!

In the last call, the delay method cannot be aware of recordset and thus,
we don't know on which ids we want to run it

We'll need to change the delay API if we want to support jobs on Model methods,
some ideas:

A. prepare_delay on BaseModel

recordset.prepare_delay(self.env['my.model'].my_method, priority=10, description='test').delay(1, k=1)

pros:

  • keep the delay() function
  • configuration of the job (priority, ...) no longer mixed with the function kwargs
  • usable with both Model methods and standalone functions

cons:

  • add a global method on Models
  • a bit confusing (what about recordset.prepare_delay(recordset.my_method, ...) or self.env['my.model'].prepare_delay(recordset.my_method, ...)?)
  • verbose, not straightforward

B. prepare_delay (or delayable?) function

from odoo.addons.queue_job.job import prepare_delay

prepare_delay(recordset.my_method, priority=10, description='test').delay(1, k=1)

pros:

  • keep the delay() function
  • configuration of the job (priority, ...) no longer mixed with the function kwargs
  • usable with both Model methods and standalone functions
  • no global method on Models

cons:

  • needs an import
  • verbose, not straightforward

C. with_delay magic method (@sebastienbeau's proposal)

recordset.with_delay(priority=10, description='test').my_method(1, k=1)
# with_delay returns a instance of a DelayableRecordset, the trick being to
# override `__getattr__` to find the method an the recordset and delay it

pros:

  • nice API, more readable than the others
  • passing from asynchronous to synhronous needs just to remove the with_delay call
  • configuration of the job (priority, ...) no longer mixed with the function kwargs

cons:

  • add a global method on Models
  • not usable with standalone functions, so we would have 2 different ways to delay jobs
  • might be a bit too magic?

@lmignon @sbidoul @sebastienbeau @lasley your opinions? any other idea?

@lmignon
Copy link
Contributor

lmignon commented Oct 6, 2016

@guewen In your exemple you put @job before @api.xxx. I think that if you want to be aware of the recordset, the @job must be put after @api. At least, for the methods decorated with @omrcache this order is important. May be I'm wrong...

@guewen
Copy link
Member Author

guewen commented Oct 6, 2016

@lmignon Okay, I didn't put any thought in this part actually, I was first interested in how we want to use the delay API. Thanks for your comment, I'll change my example.

BTW I'm not sure how the @job decorator will react with _inherit, we'll have to check that as well.

@sbidoul
Copy link
Member

sbidoul commented Oct 6, 2016

I like C.

Will it work with model methods?

Delaying standalone functions is relatively rare, and could be done with a specific method on queue.job?

@guewen
Copy link
Member Author

guewen commented Oct 6, 2016

Delaying standalone functions is relatively rare, and could be done with a specific method on queue.job?

Could be done on an AbstractModel created only for the purpose of the job too.

Will it work with model methods?

Yes, both @api.multi and @api.model would be supported.

@guewen
Copy link
Member Author

guewen commented Oct 6, 2016

Implemented C in 2794886.

Still rough in the edges and now need to be simplified with the removal of the standalone delay().

@guewen guewen force-pushed the 10.0-rework-jobs branch 2 times, most recently from bd69167 to 5821f49 Compare October 7, 2016 08:44
The next commits will remove the support of '.delay()' on standalone
methods. It will simplify the code as we now support the 2.

Standalone jobs not on a particular model will need to be done on
AbstractModel created on purpose, so we will only support jobs on
models.
The related actions are now methods of the queue.job model.
They are configured using their name instead of being passed as a
function.
@guewen
Copy link
Member Author

guewen commented Oct 7, 2016

Tests are green in queue_job. (but not in connector, which is not in the scope of this PR).
@lmignon @sbidoul do you think I should ask for a OCA/queue repository? I would like it!

@guewen
Copy link
Member Author

guewen commented Oct 7, 2016

(note: tests are green but I didn't tested for real yet)

@sbidoul
Copy link
Member

sbidoul commented Oct 7, 2016

Yes 👍 for a queue or connector-queue repository


class JobSerialized(fields.Field):
""" Serialized fields provide the storage for sparse fields. """
type = 'serialized'
Copy link
Contributor

Choose a reason for hiding this comment

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

@guewen you must chose and other type name since this value is already used by the fields.Serialized in Odoo. https://github.com/odoo/odoo/blob/10.0/odoo/fields.py#L2372
It's important to put a discriminant value for the type since it is used as key to register your field in the field's registry https://github.com/odoo/odoo/blob/10.0/odoo/fields.py#L104. This registry is used for example to fill the selection list when you add a new field with the UI....

@lasley lasley modified the milestones: 8.0, 10.0 Oct 19, 2016
This key is used to register the new field in the fields registry so it
is important to have a unique type.
@guewen
Copy link
Member Author

guewen commented Nov 2, 2016

Moved in OCA/queue#1

@danieltorres7
Copy link

Queue Job seem OK, I'm testing it working with base_import_async. But for my use cases/models connector.event is needed. So I think we also need to extract event from connector, not only in my models, but in many situations the use of events to trigger jobs is very usefull. I rewrote base_import_async depending on queue_job besides of connector, but in my case I can't progress without events.. Is there anyone working on it? Should I start this extraction, and if the answer is yes, where shoud I put the pull request?
Regards

@guewen
Copy link
Member Author

guewen commented Nov 9, 2016

@danieltorres7 please move on OCA/queue#1

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

Successfully merging this pull request may close these issues.

5 participants