-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(deletions): Fix scheduled project deletion timeout failure #103187
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
Conversation
We are seeing deletions failing on grouphash deletions on `Project.delete()`. This suggests `delete_project_group_hashses` is not deleting all hashes. Refactored this deletion to catch orphan grouphashses that might be left causing this timeout. Utilizes `delete_group_hashes` by adding a mode for deleting all group hashes of a project.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #103187 +/- ##
===========================================
+ Coverage 76.16% 80.64% +4.47%
===========================================
Files 9222 9241 +19
Lines 393800 394108 +308
Branches 25109 25052 -57
===========================================
+ Hits 299939 317823 +17884
+ Misses 93413 75823 -17590
- Partials 448 462 +14 |
…ncreasing clarity
armenzg
left a comment
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.
Discussing this in Slack.
armenzg
left a comment
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.
Good find!
| for group in RangeQuerySetWrapper( | ||
| Group.objects.filter(project_id=project_id), step=GROUP_CHUNK_SIZE | ||
| ): | ||
| groups.append(group) |
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.
This was basically restricting the deletions to rows with group being set and missing all non-group related rows.
All rows for this project are null:
SELECT count(*) FROM `getsentry.sentry_grouphash` WHERE project_id IN (4506269346365440) and group_id is null;
| )[:hashes_batch_size] | ||
| hashes_chunk = list(qs) | ||
| # Base query: all hashes for this project | ||
| qs = GroupHash.objects.filter(project_id=project_id) |
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.
This will allow querying for rows w/o having group being set.
| ) | ||
| assert len(events) == 0 | ||
|
|
||
| def test_delete_orphaned_grouphashes(self) -> None: |
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.
This test will fix with or without the fix above since we don't have the 30 second stomping during tests and the ORM deletion of Project will delete everything.
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.
oh right, you mean because actually once a project gets fully deleted the orphaned ones would get deleted anyway. I think I will instead convert it to a targeted test for delete_project_group_hashes method
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.
Good idea!
We are seeing deletions failing on grouphash deletions on `Project.delete()`. This suggests `delete_project_group_hashses` is not deleting all hashes. Refactored this deletion to catch orphan grouphashses that might be left causing this timeout. Utilizes `delete_group_hashes` by adding a mode for deleting all group hashes of a project, renaming it for simplicity.
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues: |
We are seeing deletions failing on grouphash deletions on
Project.delete(). This suggestsdelete_project_group_hashsesis not deleting all hashes. Refactored this deletion to catch orphan grouphashses that might be left causing this timeout. Utilizesdelete_group_hashesby adding a mode for deleting all group hashes of a project.