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

Limit concurrency in translations #6969

Merged
merged 10 commits into from Jun 8, 2020

Conversation

humitos
Copy link
Member

@humitos humitos commented Apr 27, 2020

It happens that projects using the same repository for all their translations
will trigger multiple builds on each commit. Adding this concurrency limitation
here helps us to protect ourselves when this happens.

@humitos humitos requested a review from Apr 27, 2020
It happens that projects using the same repository for all their translations
will trigger multiple builds on each commit. Adding this concurrency limitation
here helps us to protect ourselves when this happens.
@humitos humitos force-pushed the humitos/limit-concurrency-translations branch from 6349a2d to a31a81a Compare Apr 27, 2020
Copy link
Member

@ericholscher ericholscher left a comment

This logic is missing from the trigger_build checking I think. We should add a single place where we query this logic.

We also need to treat translations as different than the main project. Otherwise if you trigger 50 translations that take 10 minutes each, it will take 250 minutes just to process them at 2x concurrency, which is...not great.

readthedocs/api/v2/views/model_views.py Show resolved Hide resolved
Copy link
Member

@ericholscher ericholscher left a comment

This logic is missing from the trigger_build checking I think. We should add a single place where we query this logic.

We also need to treat translations as different than the main project. Otherwise if you trigger 50 translations that take 10 minutes each, it will take 250 minutes just to process them at 2x concurrency, which is...not great.

readthedocs/api/v2/views/model_views.py Outdated Show resolved Hide resolved
humitos added 3 commits Apr 27, 2020
Move the logic for the APIv2 running endpoint and `prepare_build` into one
place: BuildQuerySet, to share the logic all over the code and be able to extend
it if needed from corporate.

`/api/v2/running/` now returns,

  - limit reached
  - concurrent builds
  - max concurrent builds allowed

New logic also count translations as part of the concurrency allowed.
@humitos
Copy link
Member Author

@humitos humitos commented Apr 27, 2020

@ericholscher I put the logic all in one place (BuildQuerySet) and modify the endpoint to return more information.

I think the logic for the translations is correct and will make more sense once we have the priority queues, but we can adapt it as we wish there if needed. I currently counts "main + translations" as a whole.

@humitos
Copy link
Member Author

@humitos humitos commented Apr 30, 2020

Even if we don't want the logic around limiting translations builds (which is easy to adapt/remove), the refactor around the code to put all the login in one place is still valid/good.

Copy link
Member

@ericholscher ericholscher left a comment

I've come around on this being needed. Godot is killing our servers, and the priority celery stuff isn't working well, so I think this is the next best option.

readthedocs/projects/tasks.py Show resolved Hide resolved
@humitos humitos merged commit 2e99e78 into master Jun 8, 2020
2 checks passed
@humitos humitos deleted the humitos/limit-concurrency-translations branch Jun 8, 2020
@humitos humitos mentioned this pull request Jun 9, 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