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.

Sign up for GitHub

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

Jump to bottom

De-duplicate builds #7123

Merged
merged 12 commits into from
Jun 8, 2020
Merged

De-duplicate builds #7123

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e76a948
De-duplicate builds
humitos May 25, 2020
95350aa
Lint
humitos May 27, 2020
030b0a5
Add Build.status_code to show different user's messages
humitos Jun 3, 2020
9fcf10d
Exclude the build triggered itself from the query
humitos Jun 3, 2020
141bbf5
Add test cases for de-duplicate builds
humitos Jun 3, 2020
6ce7617
Check Build.status_code from the task
humitos Jun 3, 2020
2802da9
Make StatusCode an Enum
humitos Jun 8, 2020
e9767f1
Be more permissive when checking `Build.status_code`
humitos Jun 8, 2020
30dde39
De-duplicate builds under a Feature flag
humitos Jun 8, 2020
8ed21f6
Merge branch 'master' of github.com:readthedocs/readthedocs.org into …
humitos Jun 8, 2020
954e004
Enable feature on tests
humitos Jun 8, 2020
190a628
Use a simple constant instead of an Enum
humitos Jun 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 36 additions & 2 deletions readthedocs/core/utils/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
)
from readthedocs.doc_builder.constants import DOCKER_LIMITS
from readthedocs.projects.constants import CELERY_LOW, CELERY_MEDIUM, CELERY_HIGH
from readthedocs.doc_builder.exceptions import BuildMaxConcurrencyError
from readthedocs.doc_builder.exceptions import BuildMaxConcurrencyError, DuplicatedBuildError


log = logging.getLogger(__name__)
Expand Down Expand Up @@ -165,8 +165,42 @@ def prepare_build(
# External builds should be lower priority.
options['priority'] = CELERY_LOW

skip_build = False
if commit:
skip_build = (
Build.objects
.filter(
project=project,
version=version,
commit=commit,
).exclude(
state=BUILD_STATE_FINISHED,
).exists()
)
else:
skip_build = Build.objects.filter(
project=project,
version=version,
state=BUILD_STATE_TRIGGERED,
).count() > 1
if skip_build:
# TODO: we could mark the old build as duplicated, however we reset our
# position in the queue and go back to the end of it --penalization
humitos marked this conversation as resolved.
Show resolved Hide resolved
log.warning(
'Marking build to be skipped by builder. project=%s version=%s build=%s commit=%s',
project.slug,
version.slug,
build.pk,
commit,
)
build.error = DuplicatedBuildError.message
build.success = False
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should make this nullable and set it to None here? It didn't succeed or fail, so it kinda makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

It makes sense to me. However, I don't think we can change it now without changing the UI (knockout code) that renders in the frontend; which I'm not sure if we want to touch at this point.

Copy link
Member

Choose a reason for hiding this comment

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

Yea, I just worry this will break the badge display.

build.exit_code = 1
build.state = BUILD_STATE_FINISHED
build.save()

# Start the build in X minutes and mark it as limited
if project.has_feature(Feature.LIMIT_CONCURRENT_BUILDS):
if not skip_build and project.has_feature(Feature.LIMIT_CONCURRENT_BUILDS):
running_builds = (
Build.objects
.filter(project__slug=project.slug)
Expand Down
4 changes: 4 additions & 0 deletions readthedocs/doc_builder/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ class BuildMaxConcurrencyError(BuildEnvironmentError):
message = ugettext_noop('Concurrency limit reached ({limit}), retrying in 5 minutes.')


class DuplicatedBuildError(BuildEnvironmentError):
message = ugettext_noop('Duplicated build.')


class BuildEnvironmentWarning(BuildEnvironmentException):
pass

Expand Down
14 changes: 14 additions & 0 deletions readthedocs/projects/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
BuildEnvironmentWarning,
BuildMaxConcurrencyError,
BuildTimeoutError,
DuplicatedBuildError,
MkDocsYAMLParseError,
ProjectBuildsSkippedError,
VersionLockedError,
Expand Down Expand Up @@ -542,6 +543,19 @@ def run(
self.commit = commit
self.config = None

if all([
self.build['state'] == BUILD_STATE_FINISHED,
self.build['error'] == DuplicatedBuildError.message,
]):
log.warning(
'NOOP: build is marked as duplicated. project=%s version=%s build=%s commit=%s',
self.project.slug,
self.version.slug,
build_pk,
self.commit,
)
return True

if self.project.has_feature(Feature.LIMIT_CONCURRENT_BUILDS):
response = api_v2.build.running.get(project__slug=self.project.slug)
builds_running = response.get('count', 0)
Expand Down