Page MenuHomeSoftware Heritage

plugins: add support for scheduler's task-type declaration
ClosedPublic

Authored by douardda on Tue, Sep 3, 3:15 PM.

Details

Summary

By default, the db-init cli tool will now create missing task-type entries in the
scheduler according to:

  • only create missing task-types (do not update them), but check that the backend_name field is consistent,
  • each SWHTask-based task declared in a module listed in the 'task_modules' plugin registry field will be checked and added if needed; tasks which name start wit an underscore will not be added,
  • added task-type will have:
    • the 'type' field is derived from the task's function name (with underscores replaced with dashes),
    • the description field is the first line of that function's docstring,
    • default values as provided by the swh.lister.cli.DEFAULT_TASK_TYPE (with a simple pattern matching to have decent default values for full/incremental tasks),
    • these default values can be overloaded via the 'task_type' plugin registry entry.

For this, we had to rename all tasks names (eg. cran_lister -> list_cran).

Diff Detail

Repository
rDLS Listers
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

douardda created this revision.Tue, Sep 3, 3:15 PM
vlorentz added inline comments.
swh/lister/npm/__init__.py
13–19

why a specific value for npm?
There should be at least a comment about this.

ardumont added inline comments.
swh/lister/cli.py
140

I like this but should this be called in the db-init subcommand?

That will work for the docker-dev environment but what about production?

145

key does not convey much ;)
I had to scratch my head a bit to understand it was the lister's "listing property" (full, incremental)...
I don't know how to call it though.

swh/lister/npm/__init__.py
13–19

Probably because that matches the current production setup.

               type                |                         description                          |                     backend_name                      | default_interval | min_interval | max_interval | backoff_factor | max_queue_length$
-----------------------------------+--------------------------------------------------------------+-------------------------------------------------------+------------------+--------------+--------------+----------------+-----------------$
 list-npm-full                     | Full npm lister                                              | swh.lister.npm.tasks.NpmListerTask                    | 7 days           | 7 days       | 7 days       |              1 |                 $

Thanks /me likes!

I just have an issue about how we integrate this for production (we might not change a thing on our m.o., i'm just unsure ;)

vlorentz added inline comments.Tue, Sep 3, 3:50 PM
swh/lister/npm/__init__.py
13–19

And why was it done in production?

ardumont added inline comments.Tue, Sep 3, 3:55 PM
swh/lister/npm/__init__.py
13–19

I don't really know (i must have kept the existing ones which we now find somehow consistently in the scheduling db).

Same goes for the existing other setup, why 90 (list-full), 64 (loaders), 1 (list-incremental) day(s)?
That probably sounded reasonable at some point in time.

douardda added inline comments.Tue, Sep 3, 3:58 PM
swh/lister/npm/__init__.py
13–19

Probably because that matches the current production setup.

yep!

douardda added inline comments.Tue, Sep 3, 4:02 PM
swh/lister/cli.py
140

I am not yet sure where I want for this new feature/command to live.

It might make sense to make it a dedicated command indeed (possibly called from db-init).

For the production system, I guess one would prefer this 'upgrade' task to be done by hand, so I won't make it executed automagically somehow.

145

agreed, could use something like pattern (but it's not a regex) of suffix maybe?

ardumont added inline comments.Tue, Sep 3, 4:09 PM
swh/lister/cli.py
140

It might make sense to make it a dedicated command indeed (possibly called from db-init).

Yes (i thought of that \m/ ;)

subcommand: register-task or something?

For the production system, I guess one would prefer this 'upgrade' task to be done by hand, so I won't make it executed automagically somehow.

yes, i'd prefer that the call be manual (at least at first ;)

(Also, you made it idempotent already so that should be fine to use)

145

yes, suffix sounds good.

Might be adding a comment to explicit the suffix lives in a fixed set of {'full', 'incremental'} (as of now ;)

ardumont accepted this revision.Tue, Sep 3, 5:11 PM

To be clear, i'm fine with the diff's goal and how it's implemented.

It'd be great to have what we exchanged about but that could be iterated over in another diff.

Also, the ci build failure seems to be irrelevant to the diff (patch appliance failure so meh, maybe a missing rebase or some such).

This revision is now accepted and ready to land.Tue, Sep 3, 5:11 PM
douardda updated this revision to Diff 6554.Wed, Sep 4, 11:02 AM

attempt to answer ardumont and vlorents queries/comments

ardumont accepted this revision.Wed, Sep 4, 11:12 AM
ardumont added inline comments.
swh/lister/cli.py
142

According to

164

you could make that a """ docstring """ ;)

douardda updated this revision to Diff 6559.Wed, Sep 4, 11:51 AM

refactor a bit the ensure_task_type function and write a docstring

and fix a typo repported by ardumont

douardda updated this revision to Diff 6583.Wed, Sep 4, 3:39 PM

Add tests, and (as a consequence) a fix

This revision was automatically updated to reflect the committed changes.