Skip to content

fix(workflow): Add locks to groupowners to prevent duplicates #29309

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

Merged
scttcper merged 4 commits into from
Oct 19, 2021
Merged

fix(workflow): Add locks to groupowners to prevent duplicates #29309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
afa1908
add locks to groupowners
scttcper Oct 14, 2021
76e1c91
wrap entire functions
scttcper Oct 14, 2021
9567f8f
combine locks
scttcper Oct 19, 2021
75e89e8
Merge branch 'master' into scttcper/lock-groupowners
scttcper Oct 19, 2021
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
76 changes: 50 additions & 26 deletions src/sentry/tasks/groupowner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@

from django.utils import timezone

from sentry.app import locks
from sentry.models import Commit, Project, Release
from sentry.models.groupowner import GroupOwner, GroupOwnerType
from sentry.tasks.base import instrumented_task
from sentry.utils import metrics
from sentry.utils.committers import get_event_file_committers
from sentry.utils.locking import UnableToAcquireLock
from sentry.utils.sdk import set_current_event_project

PREFERRED_GROUP_OWNERS = 2
Expand All @@ -17,13 +19,9 @@
logger = logging.getLogger("tasks.groupowner")


@instrumented_task(
name="sentry.tasks.process_suspect_commits",
queue="group_owners.process_suspect_commits",
default_retry_delay=5,
max_retries=5,
)
def process_suspect_commits(event_id, event_platform, event_frames, group_id, project_id, **kwargs):
def _process_suspect_commits(
event_id, event_platform, event_frames, group_id, project_id, **kwargs
):
metrics.incr("sentry.tasks.process_suspect_commits.start")
set_current_event_project(project_id)

Expand Down Expand Up @@ -66,25 +64,34 @@ def process_suspect_commits(event_id, event_platform, event_frames, group_id, pr
for owner_id in sorted(owner_scores, reverse=True, key=owner_scores.get)[
:PREFERRED_GROUP_OWNERS
]:
go, created = GroupOwner.objects.update_or_create(
group_id=group_id,
type=GroupOwnerType.SUSPECT_COMMIT.value,
user_id=owner_id,
project=project,
organization_id=project.organization_id,
defaults={
"date_added": timezone.now()
}, # Updates date of an existing owner, since we just matched them with this new event
)
if created:
owner_count += 1
if owner_count > PREFERRED_GROUP_OWNERS:
try:
owner = owners[0]
except IndexError:
pass
else:
owner.delete()
try:
go, created = GroupOwner.objects.update_or_create(
group_id=group_id,
type=GroupOwnerType.SUSPECT_COMMIT.value,
user_id=owner_id,
project=project,
organization_id=project.organization_id,
defaults={
"date_added": timezone.now()
}, # Updates date of an existing owner, since we just matched them with this new event
)
if created:
owner_count += 1
if owner_count > PREFERRED_GROUP_OWNERS:
try:
owner = owners[0]
except IndexError:
pass
else:
owner.delete()
except GroupOwner.MultipleObjectsReturned:
GroupOwner.objects.filter(
group_id=group_id,
type=GroupOwnerType.SUSPECT_COMMIT.value,
user_id=owner_id,
project=project,
organization_id=project.organization_id,
)[0].delete()

except Commit.DoesNotExist:
logger.info(
Expand All @@ -96,3 +103,20 @@ def process_suspect_commits(event_id, event_platform, event_frames, group_id, pr
"process_suspect_commits.skipped",
extra={"event": event_id, "reason": "no_release"},
)


@instrumented_task(
name="sentry.tasks.process_suspect_commits",
queue="group_owners.process_suspect_commits",
default_retry_delay=5,
max_retries=5,
)
def process_suspect_commits(event_id, event_platform, event_frames, group_id, project_id, **kwargs):
lock = locks.get(f"process-suspect-commits:{group_id}", duration=10)
try:
with lock.acquire():
_process_suspect_commits(
event_id, event_platform, event_frames, group_id, project_id, **kwargs
)
except UnableToAcquireLock:
pass
78 changes: 41 additions & 37 deletions src/sentry/tasks/post_process.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,45 +131,49 @@ def handle_group_owners(project, group, owners):
from sentry.models.team import Team
from sentry.models.user import User

with metrics.timer("post_process.handle_group_owners"):
current_group_owners = GroupOwner.objects.filter(
group=group, type=GroupOwnerType.OWNERSHIP_RULE.value
)
new_owners = {(type(owner), owner.id) for owner in owners}
# Owners already in the database that we'll keep
keeping_owners = set()
for owner in current_group_owners:
lookup_key = (
(Team, owner.team_id) if owner.team_id is not None else (User, owner.user_id)
lock = locks.get(f"groupowner-bulk:{group.id}", duration=10)
try:
with lock.acquire(), metrics.timer("post_process.handle_group_owners"):
current_group_owners = GroupOwner.objects.filter(
group=group, type=GroupOwnerType.OWNERSHIP_RULE.value
)
if lookup_key not in new_owners:
owner.delete()
else:
keeping_owners.add(lookup_key)

new_group_owners = []

for key in new_owners:
if key not in keeping_owners:
owner_type, owner_id = key
user_id = None
team_id = None
if owner_type is User:
user_id = owner_id
if owner_type is Team:
team_id = owner_id
new_group_owners.append(
GroupOwner(
group=group,
type=GroupOwnerType.OWNERSHIP_RULE.value,
user_id=user_id,
team_id=team_id,
project=project,
organization=project.organization,
)
new_owners = {(type(owner), owner.id) for owner in owners}
# Owners already in the database that we'll keep
keeping_owners = set()
for owner in current_group_owners:
lookup_key = (
(Team, owner.team_id) if owner.team_id is not None else (User, owner.user_id)
)
if new_group_owners:
GroupOwner.objects.bulk_create(new_group_owners)
if lookup_key not in new_owners:
owner.delete()
else:
keeping_owners.add(lookup_key)

new_group_owners = []

for key in new_owners:
if key not in keeping_owners:
owner_type, owner_id = key
user_id = None
team_id = None
if owner_type is User:
user_id = owner_id
if owner_type is Team:
team_id = owner_id
new_group_owners.append(
GroupOwner(
group=group,
type=GroupOwnerType.OWNERSHIP_RULE.value,
user_id=user_id,
team_id=team_id,
project=project,
organization=project.organization,
)
)
if new_group_owners:
GroupOwner.objects.bulk_create(new_group_owners)
except UnableToAcquireLock:
pass


def update_existing_attachments(event):
Expand Down