-
Notifications
You must be signed in to change notification settings - Fork 12
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
Retry aggregate_assets_summary_task on version metadata race condition #1803
Conversation
326ab72
to
9dc252f
Compare
9dc252f
to
9bc1979
Compare
@@ -95,6 +96,7 @@ def version_aggregate_assets_summary(version: Version) -> None: | |||
) | |||
if updated_count == 0: | |||
logger.info('Skipped updating assetsSummary for version %s', version.id) | |||
raise VersionMetadataConcurrentlyModified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this trigger a Sentry error? I imagine we wouldn't want that to happen in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it will. I believe what happens is the following:
- This exception is raised
- Since the task is decorated with
autoretry_for
, it catches that exception and raises a celeryRetry
exception (documented here) - Sentry (specific the celery integration) sees the
Retry
exception is raised, but ignores it, as it's recognized as part of celery's control flow. Here is the code that controls that.
So we should be good, but I'll do a little more testing to make sure things behave as we expect before merging.
🚀 PR was released in |
Fixes #1734
Related to #1800. Both this PR and #1800 address #1734, although this change should cover all sources of a concurrent metadata change, instead of just the
validate_version_metadata
task.