Page MenuHomeSoftware Heritage

Explicitly register class-based tasks inheriting from our own class
ClosedPublic

Authored by olasd on Dec 18 2018, 5:32 PM.

Details

Summary

This lets go of the metaclass madness imported from Celery 3, and goes for an
explicit task registration mechanism as advised in Celery 4.

We do explicit registration of all our tasks in the 'worker ready' signal, just
before they automatically subscribe to the task queues.

Unfortunately that signal is not emitted by the "test fixture" worker, so we
need to explicitly register class-based tasks that are being used in tests. This
doesn't show up for functions as the @task decorator handles registration.

Test Plan

same tests as D852: tox, worker with custom config, plain worker with -I <modules>

Diff Detail

Repository
rDSCH Scheduling utilities
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 3189
Build 4087: tox-on-jenkinsJenkins
Build 4086: arc lint + arc unit

Event Timeline

olasd created this revision.Dec 18 2018, 5:32 PM

We could keep the metaclass to set cls.name, instead of defining it in every class.

Or maybe migrate to using functions decorated with @app.task.

swh/scheduler/celery_backend/config.py
6

Could you just import itertools? chain is also used by celery (which recommends using from celery import chain) so it got me confused for a second when reading the code below

84–114

Adding a docstring to this function would be nice, it's getting non-trivial.

olasd marked 2 inline comments as done.Dec 18 2018, 6:03 PM

We could keep the metaclass to set cls.name, instead of defining it in every class.

eww :(

I think the few places we need to explicitly set the name (basically, in tests, and when we register the class) means the metaclass would not really save that much.

Or maybe migrate to using functions decorated with @app.task.

We currently use inheritance in a bunch of places to create "task patterns" (the most obvious use of this is in the lister). I agree that's something we c/should reconsider, but for now I focused on making the inter-module diff minimal (only the lister needs a change because the metaclass has been dropped)

swh/scheduler/celery_backend/config.py
84–114

Yes, agreed.

olasd updated this revision to Diff 2728.Dec 18 2018, 6:04 PM

Apply @vlorentz's comments

olasd marked 2 inline comments as done.Dec 18 2018, 6:04 PM
ardumont accepted this revision.Dec 19 2018, 12:40 PM
This revision is now accepted and ready to land.Dec 19 2018, 12:40 PM