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

Tasks routing #1487

Merged
merged 3 commits into from Mar 8, 2018
Merged

Tasks routing #1487

merged 3 commits into from Mar 8, 2018

Conversation

noirbizarre
Copy link
Contributor

This PR refactor the task routing: the default route can be defined at declaration time, on both task and job, by either using:

  • queue and/or routing_key parameters
  • route parameter from which queue and routing_key will be computed (route=high.somewhere gives queue=high and routing_key=high.somewhere)

This also allows extension to define routing on their tasks.

Some route have been added to the existing tasks and jobs.

@noirbizarre noirbizarre added this to the 1.3.0 milestone Mar 8, 2018
@noirbizarre noirbizarre self-assigned this Mar 8, 2018
@noirbizarre noirbizarre requested a review from a team March 8, 2018 08:56
@@ -121,7 +121,7 @@ def get_i18n_analyzer():
i18n_analyzer = LocalProxy(lambda: get_i18n_analyzer())


@celery.task
@task(queue='high', routing_key='high.search')
Copy link
Contributor

Choose a reason for hiding this comment

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

why not route='high.search'? To stay consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just forgot to remove the queue. I added the route after as a shortcut.

@@ -26,6 +29,7 @@ def __call__(self, *args, **kwargs):

class JobTask(ContextTask):
abstract = True
default_queue = 'low'
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe default? Because, you know, default ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is default for tasks, jobs are meant to be long running tasks this is why I set them to low by default. But I can undo this, but this mean we have to declare queue=low manually for every job

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes senses 👍

@job('test-low-queue', queue='low')
@job('test-default-queue', queue='default')
@job('test-high-queue', queue='high')
def queue_test(self, raises=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

can't be defined in test files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is intentional, like log-test and error-test: this allow to manually run these jobs with udata job run -d test-high-queue and see if this is properly routing.
You can also run udata job run -d test-high-queue raises=True and you can have the time for a tasks to be processed because the exception will appear in Sentry with a timestamp.

@noirbizarre noirbizarre merged commit 5abfe35 into opendatateam:master Mar 8, 2018
@noirbizarre noirbizarre deleted the tasks-routing branch March 8, 2018 09:37
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.

None yet

2 participants