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

Add basic support for lower priority PR builds #6921

Merged
merged 8 commits into from Apr 27, 2020
Merged

Conversation

ericholscher
Copy link
Member

@ericholscher ericholscher commented Apr 17, 2020

To test this I added this to our tasks:

@app.task(ignore_result=True)
def priority_test(priority):
    import time
    time.sleep(1)
    print(priority)

Then ran this script in a manage.py shell_plus:

from readthedocs.projects.tasks import priority_test

for i in range(50):
    priority_test.apply_async((i%10,), queue="default", priority=i%10)

from redis import Redis
r = Redis.from_url(settings.BROKER_URL)
for k in r.keys('default*'):
    print(k.decode())
    print(r.llen(k))
    #print(r.delete(k))

@ericholscher ericholscher requested a review from Apr 17, 2020
@ericholscher ericholscher marked this pull request as draft Apr 17, 2020
readthedocs/core/utils/__init__.py Outdated Show resolved Hide resolved
readthedocs/settings/base.py Outdated Show resolved Hide resolved
@humitos
Copy link
Member

@humitos humitos commented Apr 17, 2020

The example shows it works locally. It puts 20 tasks on each queue (it shows 19 because I suppose 2 are consumed immediately by the workers and they are gone when it checks redis). After that, all the tasks with Priority 1 are consumed first and then all the tasks with priority 0.

@ericholscher
Copy link
Member Author

@ericholscher ericholscher commented Apr 17, 2020

@humitos I cleaned up the logic around it now that we have it working.

@ericholscher ericholscher marked this pull request as ready for review Apr 17, 2020
Copy link
Member

@humitos humitos left a comment

Happy to have this working! I like the idea of having 3 priorities.

I'm a little confused about how this works (I may need to read more the documentation :) ), but in the meanwhile: do you know if these priorities apply to all the queues by default? I mean, will we end up with N queues * 3? Will workers automatically listen on those queues or do we need to update them to listen in build:default{1,2,3} for example? (which I haven't in my local test, BTW)

# Set 3 priorities, [low, medium, high] -- default is medium
# Leave some space on each side of the set to expand if needed
CELERY_LOW = 3
CELERY_MEDIUM = 5
CELERY_HIGH = 7
Copy link
Member

@humitos humitos Apr 17, 2020

Choose a reason for hiding this comment

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

nit: maybe these fit better in builds.contants?

@ericholscher ericholscher requested a review from Apr 27, 2020
@@ -150,6 +151,11 @@ def prepare_build(
# Send Webhook notification for build triggered.
send_notifications.delay(version.pk, build_pk=build.pk, email=False)

options['priority'] = CELERY_HIGH
if version.type == EXTERNAL:
Copy link
Member

@humitos humitos Apr 27, 2020

Choose a reason for hiding this comment

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

Want we check for being a translation here before checking if external and assigning CELERY_MEDIUM?

@@ -169,6 +171,7 @@ def test_trigger_build_rounded_time_limit(self, update_docs):
'queue': mock.ANY,
'time_limit': 3,
'soft_time_limit': 3,
'priority': CELERY_HIGH,
Copy link
Member

@humitos humitos Apr 27, 2020

Choose a reason for hiding this comment

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

We should modify/add a test that triggers a PR and check it's CELERY_LOW.

@ericholscher
Copy link
Member Author

@ericholscher ericholscher commented Apr 27, 2020

Addressed review feedback, going to ship this and QA this week 👍

@ericholscher ericholscher merged commit 5262fc0 into master Apr 27, 2020
2 checks passed
@ericholscher ericholscher deleted the external-priority branch Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants