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

Conversation

Projects
None yet
2 participants
@humitos
Member

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: rtfd/readthedocs-ops#392 (comment)

@humitos humitos requested a review from rtfd/core Aug 21, 2018

@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Aug 21, 2018

Member

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.

Member

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.

Route task to proper queue
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.
@ericholscher

ericholscher approved these changes Aug 22, 2018 edited

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

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Aug 22, 2018

Member

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.

Member

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

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@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