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

Have the same lock for upload + preprocess task #502

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 6 additions & 2 deletions tasks/preprocess_upload.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
from services.yaml import save_repo_yaml_to_database_if_needed
from services.yaml.fetcher import fetch_commit_yaml_from_provider
from tasks.base import BaseCodecovTask
from tasks.upload import UPLOAD_LOCK_NAME

log = logging.getLogger(__name__)

Expand All @@ -48,7 +49,7 @@ def run_impl(
"Received preprocess upload task",
extra=dict(repoid=repoid, commit=commitid, report_code=report_code),
)
lock_name = f"preprocess_upload_lock_{repoid}_{commitid}_{report_code}"
lock_name = UPLOAD_LOCK_NAME(repoid, commitid)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be useful to add a comment explaining why this task needs the same lock as the upload task

redis_connection = get_redis_connection()
# This task only needs to run once per commit (per report_code)
# To generate the report. So if one is already running we don't need another
Expand All @@ -62,7 +63,10 @@ def run_impl(
with redis_connection.lock(
lock_name,
timeout=60 * 5,
blocking_timeout=None,
# This is the timeout that this task will wait to wait for the lock. This should
# be non-zero as otherwise it waits indefinitely to get the lock. It's also smaller than
# the upload task's blocking timeout to guarantee an Upload tasks runs
blocking_timeout=3,
):
return self.process_impl_within_lock(
db_session=db_session,
Expand Down
6 changes: 5 additions & 1 deletion tasks/upload.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@

CHUNK_SIZE = 3

# Making the upload lock name a constant so it can be shared between the preprocess and
# the upload task, as they have overlapping logic that collides in some cases
UPLOAD_LOCK_NAME = lambda repoid, commitid: f"upload_lock_{repoid}_{commitid}"


class UploadContext:
"""
Expand Down Expand Up @@ -90,7 +94,7 @@ def lock_name(self, lock_type: str):
if lock_type == "upload_processing":
return UPLOAD_PROCESSING_LOCK_NAME(self.repoid, self.commitid)
else:
return f"{lock_type}_lock_{self.repoid}_{self.commitid}"
return UPLOAD_LOCK_NAME(self.repoid, self.commitid)
else:
return f"{lock_type}_lock_{self.repoid}_{self.commitid}_{self.report_type.value}"

Expand Down
Loading