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

Route task to proper queue #4553

Merged
merged 2 commits into from Aug 22, 2018
Merged

Route task to proper queue #4553

merged 2 commits into from Aug 22, 2018

Conversation

@humitos
Copy link
Member

@humitos humitos commented Aug 21, 2018

In #4033, I introduce a bug when using Celery signatures.

From Celery's docs at http://docs.celeryproject.org/en/latest/reference/celery.html#celery.signature

the .s() shortcut does not allow you to specify execution options but there’s a chaning .set method that returns the signature

So, instead of dealing with multiple .set(), I'm just using the .signature() method of the task which is more explicit.

Ref: readthedocs/readthedocs-ops#392 (comment)

@humitos humitos requested a review from Aug 21, 2018
@humitos
Copy link
Member Author

@humitos humitos commented Aug 21, 2018

I tested this locally by running an extra celery worker:

celery -A readthedocs worker -n build03 -Q build03 -l DEBUG -c 2

and setting build03 as the queue for the project to be built.

In #4033, I introduce a bug when using Celery signatures.

From Celery's docs at http://docs.celeryproject.org/en/latest/reference/celery.html#celery.signature

> the .s() shortcut does not allow you to specify execution options
  but there’s a chaning .set method that returns the signature

So, instead of dealing with multiple `.set()`, I'm just using the
`.signature()` method of the task which is more explicit.
@humitos humitos force-pushed the humitos/celery/queue-routing branch from 3d83afa to d873986 Aug 21, 2018
Copy link
Member

@ericholscher ericholscher left a comment

Looks good except for the failing tests.

Is there a way to test this, so it doesn't break in the future? It seems like we're just testing the mock call, but not that it's the proper call to celery. Hrm.

@humitos
Copy link
Member Author

@humitos humitos commented Aug 22, 2018

I'm not sure if there is a better way to test this. I think the problem with this was that I modified/wrote invalid tests for this. Now, I added a test that checks that the specific queue used in project.build_queue is the one passed to the task's signature.

Still, this is not perfect, but I suppose we are a little safer now with this test.

@humitos humitos merged commit 4d8571a into master Aug 22, 2018
1 check passed
@humitos humitos deleted the humitos/celery/queue-routing branch Aug 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants