novacode-nl / odoo-celery

Odoo & Celery integration
GNU Lesser General Public License v3.0
42 stars 44 forks source link

Deletion of old tasks #10 #11

Closed litnimax closed 4 years ago

litnimax commented 5 years ago

Hello! Is this code ok for the ticket #10? ;-)

bobslee commented 5 years ago

Hi @litnimax Sorry for my late reply. Really thank you for the PR! It looks really good and useful!

May I propose to improve this further? Please let me know whether you agree or it seems feasible. Otherwise I would like to merge your PR anyway!

Proposal

It would be very useful to configure this cleanup per Task. Which is also beneficial to exclude some tasks from the cleanup, which may not be deleted or after a very long period. The latest Celery version already contains the celery.task.setting model, see: https://github.com/novacode-nl/odoo-celery/blob/12.0/celery/models/celery_task_setting.py

Development

  1. Add 2 fields to the celery.task.setting model and form view (pseudo code):
    • vacuum_interval = fields.Integer
    • vacuum_period_time = fields.Selection([seconds, minutes, hours, days])

Example in form view: https://imagebin.ca/v/4icc0myaYw2d

  1. Create a transient/report model (celery.vacuum.task) which queries the Tasks for deletion/vacuum. For example see: celery.jammed.task.report

The query use the to fields from the celery.task.setting (step 1 above), to construct the timedelta in SQL.

  1. Remove the time intervals from the Cron method arguments.
  2. Change cron method, to query the celery.vacuum.task.
bobslee commented 5 years ago

It would also be an improvement to configure which Task state shall be searched to delete. So for example Tasks in state FAILURE won't be deleted for certain tasks.

litnimax commented 5 years ago

Hi. I don't mind doing some more coding on this. Just need to be sure I got you right. You want to have tasks with different vacuum settings (ok). So on task create one can set task attributes related to vacuum (ok). I did not understand how task.vacuum_interval can be used as interval is a cron setting on how often to call vacuum. Also I don't understand why transient model is needed. We can just have a cron method calls at specified interval (in args) and this method will query tasks that are older then task's "age" attribute (aha seems that your task.vacuum_interval is this setting). Looking forward to your comments. Regards, Max.

litnimax commented 5 years ago

Also a question: what to do with "lost" tasks? By "lost" I mean tasks in states STARTED, RETRY that will never be executed. Even if I enable unlink perm I still cannot delete it due to unlink() method restriction:

@api.multi
    def unlink(self):
        for task in self:
            if task.state in [STATE_STARTED, STATE_RETRY]:
                raise UserError(_('You cannot delete a running task.'))
        super(CeleryTask, self).unlink()

Screenshot

bobslee commented 5 years ago

Regarding the "lost" tasks: There's already functionality to handle those kind of tasks, called "Jammed Tasks". Unfortunately not documented yet, but I shall explain below.

I shall test your PR regarding the vacuum Cron and come back to you whether to merge. I'm currently in a lack of time to elaborate about my proposed improvements or any progress on this module.

Jammed Tasks

Jammed task settings

Tasks that seem Jammed

Handle Tasks in state Jammed

So finally you're able to delete (or Cron vacuum) the Cancelled tasks :)

Note: I know there's some work going on about Jammed and Failed tasks notifications (mail, web popup). So stay tuned ;)

bobslee commented 5 years ago

Hi Max, See my answers.

I don't mind doing some more coding on this.

Would be great!

You want to have tasks with different vacuum settings (ok).

Yes.

So on task create one can set task attributes related to vacuum (ok).

No please not on the task create i.e. call_task method. We need to be more flexibel and able to adjust those vacuum settings afterwards, for tasks still in the queue. You understand?

I did not understand how task.vacuum_interval can be used as interval is a cron setting on how often to call vacuum. Also I don't understand why transient model is needed.

Indeed the transient model isn't needed for this. However I would like to postpone this feature with vacuum intervals by task setting. I know what to do and shall develop when I have some free time.

We can just have a cron method calls at specified interval (in args) and this method will query tasks that are older then task's "age" attribute (aha seems that your task.vacuum_interval is this setting).

Please could you change the cron method arguments into kwargs? So we can ensure future changes won't break compatibility.

Could you also document all current kwargs (parameters) which can be given to the cron method? Document e.g. as:

Looking forward for your contribution :)

bobslee commented 4 years ago

@litnimax I just merged this PR.

Heads up: I also changed the cron_autovacuum function to use kwargs instead of positional arguments.

So be careful - change the arguments - in case you merge/update from this upstream repository again.

Reasons for kwargs:

Thanks again for your improvement. Really appreciated!

bobslee commented 4 years ago

@litnimax I also renamed the function from cron_autovacuum to autovacuum.

litnimax commented 4 years ago

Thanks man for update. Though I am now quite apart from Odoo Celery project (in love with nameko.io).

bobslee commented 4 years ago

You're welcome Max :) Glad you found a new useful and inspiring framework. Looks promising! Ofcourse it's a different kinda beast then Odoo, focussing on ERP, RAD framework and batteries (modules) included ;) Wish you the best!