chore(hybrid-cloud): Break queries that depend on cross silo FKs by corps · Pull Request #45095 · getsentry/sentry

corps · 2023-02-24T20:39:35Z

Separating out the query changes from https://github.com/getsentry/sentry/pull/44595/files

Hybrid Cloud needs to break many foreign key relationships that depend on cross silo models. To support this, we have a new column which acts merely as a big int referencing identifiers, but uses a eventually consistent system to cascade or set null when deletions happen across silos.

This PR first changes all known api and test usages of several cross silo foreign keys in preparation for the migration that actually breaks those foreign keys. Several more to come.

markstory · 2023-02-27T16:48:54Z

src/sentry/api/serializers/models/alert_rule.py

+        )

-            result[alert_rules[rule_activity.alert_rule.id]].update({"created_by": user})
+        rule_activities_by_user_id = {r.user_id: r for r in rule_activities}


Is there only one activity per user? Or could a user have multiple activities?

Good point, and I'll adjust

markstory · 2023-02-27T16:50:16Z

src/sentry/api/serializers/models/exporteddata.py

+        serialized_users = {
+            u["id"]: u
+            for u in user_service.serialize_many(
+                filter=dict(user_ids=[item.user_id for item in item_list])
+            )
+        }


We seem to do this a bunch. Should the user_service expose a serialize_many_map method?

In, general, I think the serialize_many should be returning a dictionary, so I agree. I had been thinking about making a larger refactor for this purpose.

I'd still prefer to keep the service interfaces thing (one way of doing things, not 3), but in this case returning by dictionary is actually preferable to the list, so I may replace it that way.

I'll make some follow up work to consider refactoring all serialize_many call sites into this convention.

markstory · 2023-02-27T16:52:04Z

src/sentry/api/serializers/models/group.py

+                all_team_ids[g.team_id] = g.group_id
+            if g.user_id:
+                all_user_ids[g.user_id] = g.group_id


Do teams only have a single group? If item_list is a collection of groups, couldn't a team be assigned to multiple groups?

Yes, I think you may be right. I'll take a closer look to see if this can be simplified. In general, I wish we had a much more robust hybrid cloud actor interface.

markstory · 2023-02-27T16:55:46Z

src/sentry/tasks/integrations/slack/find_channel_id_for_rule.py

+                except IntegrityError:
+                    pass


Was the integrity error pre-existing and now you're just handling it?

corps · 2023-02-27T17:15:41Z

src/sentry/tasks/integrations/slack/find_channel_id_for_rule.py

        redis_rule_status.set_value("failed")
        return

-    user = None


@markstory
Previously, this check helped inform the RuleActivity check below. Using IntegrityError captures the same meaning but without the lookup. In the future, when this fk is broken for real, it will become a service call and no other enforcement. This was the intermediate until the FK is broken because technically an integrity error can be raised so long as it remains.

markstory

Generally looks good to me. I found a few more incorrect passages and the CI failures seem relevant as well.

markstory · 2023-02-28T03:11:38Z

src/sentry/api/serializers/models/alert_rule.py

+        use_by_user_id: MutableMapping[int, RpcUser] = {
+            user.id: user
+            for user in user_service.get_many(
+                filter=dict(user_ids=[r.user_id for r in rule_activities])


When we get around to building RPC calls we should make sure we only send unique userids in the request body.

Yeah, ideally our service interfaces use "wide" types like Sequence so that I don't have to force a list. This is on my mind, too.

markstory · 2023-02-28T03:15:28Z

src/sentry/api/serializers/models/group.py

+        all_team_ids: MutableMapping[int, Set[int]] = {}
+        all_user_ids: MutableMapping[int, Set[int]] = {}


nit: You could make these defaultdict() to save the conditions on 193 and 199

markstory · 2023-02-28T03:16:04Z

src/sentry/api/serializers/models/group.py

+                else:
+                    all_team_ids[g.team_id].add(g.group_id)
+            if g.user_id:
+                if g.team_id not in all_team_ids:


Suggested change

if g.team_id not in all_team_ids:

if g.user_id not in all_user_ids:

markstory · 2023-02-28T03:21:48Z

src/sentry/data_export/tasks.py

-                if data_export.user.email:
-                    user["email"] = data_export.user.email
+            if data_export.user_id:
+                user = dict(id=data_export.user_id)


Trimming down to only user_id seems reasonable as getting the user will use another RPC call.

markstory · 2023-02-28T03:24:58Z

tests/sentry/integrations/slack/test_unfurl.py

    assert match_link(url) == expected


+# @region_silo_test(stable=True)


Suggested change

# @region_silo_test(stable=True)

getsentry-bot · 2023-02-28T19:30:50Z

PR reverted: da6cf99

…FKs (#45095)" This reverts commit 2d87621. Co-authored-by: corps <593850+corps@users.noreply.github.com>

I need to remerge #45095 after reverting prematurely.

chore(hybrid-cloud): Break queries that depend on cross silo FKs

22f3973

corps requested a review from a team February 24, 2023 20:39

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Feb 24, 2023

vercel bot deployed to Preview February 24, 2023 20:41 View deployment

Removing accidental cherry pick

62d82ea

vercel bot deployed to Preview February 24, 2023 20:52 View deployment

Thingy

83753e0

vercel bot deployed to Preview February 24, 2023 20:55 View deployment

Minor fixes

786887e

corps marked this pull request as ready for review February 24, 2023 22:30

corps requested review from a team February 24, 2023 22:30

corps requested a review from a team as a code owner February 24, 2023 22:30

Merge branch 'master' into zc/break-fks-query-part

1f6109f

vercel bot deployed to Preview February 24, 2023 22:34 View deployment

Merge branch 'master' into zc/break-fks-query-part

cab3e82

vercel bot deployed to Preview February 27, 2023 16:50 View deployment

markstory reviewed Feb 27, 2023

View reviewed changes

corps commented Feb 27, 2023

View reviewed changes

Addressing feedback

8b99905

vercel bot deployed to Preview February 27, 2023 22:36 View deployment

Fix

0bc0e16

vercel bot deployed to Preview February 27, 2023 23:08 View deployment

markstory approved these changes Feb 28, 2023

View reviewed changes

Fix

e4b00c9

vercel bot deployed to Preview February 28, 2023 16:20 View deployment

corps merged commit 2d87621 into master Feb 28, 2023

corps deleted the zc/break-fks-query-part branch February 28, 2023 19:29

corps added the Trigger: Revert Add to a merged PR to revert it (skips CI) label Feb 28, 2023

corps restored the zc/break-fks-query-part branch February 28, 2023 19:30

getsentry-bot added a commit that referenced this pull request Feb 28, 2023

Revert "chore(hybrid-cloud): Break queries that depend on cross silo …

da6cf99

…FKs (#45095)" This reverts commit 2d87621. Co-authored-by: corps <593850+corps@users.noreply.github.com>

corps mentioned this pull request Feb 28, 2023

chore(hybrid-cloud): breaking foreign keys #45203

Merged

corps added a commit that referenced this pull request Feb 28, 2023

chore(hybrid-cloud): breaking foreign keys (#45203)

ede2640

I need to remerge #45095 after reverting prematurely.

github-actions bot locked and limited conversation to collaborators Mar 16, 2023

		all_team_ids: MutableMapping[int, Set[int]] = {}
		all_user_ids: MutableMapping[int, Set[int]] = {}

	if g.team_id not in all_team_ids:
	if g.user_id not in all_user_ids:

		assert match_link(url) == expected


		# @region_silo_test(stable=True)

Uh oh!

Conversation

corps commented Feb 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markstory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

getsentry-bot commented Feb 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants