Skip to content

Conversation

@snigdhas
Copy link
Contributor

Some issues have the same regression/resolution time in our db and the range boundary today would reject those as valid start/end times for open periods. Updating the boundary to be inclusive on both ends should fix this and allow an open period to start at the same time that the previous one ends.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jun 13, 2025
@snigdhas snigdhas marked this pull request as ready for review June 13, 2025 18:06
@snigdhas snigdhas requested a review from a team as a code owner June 13, 2025 18:06
@snigdhas snigdhas requested review from a team and wedamija June 13, 2025 18:06
@codecov
Copy link

codecov bot commented Jun 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #93535    +/-   ##
========================================
  Coverage   88.03%   88.04%            
========================================
  Files       10325    10330     +5     
  Lines      595677   596288   +611     
  Branches    23134    23134            
========================================
+ Hits       524411   524989   +578     
- Misses      70773    70806    +33     
  Partials      493      493            

@snigdhas
Copy link
Contributor Author

Hm ADD CONSTRAINT EXCLUDE is an unsafe operation -- we can wipe the table and pause writes before applying this if that would make it safe? Otherwise the safer suggestion is to make a new table 🤔

@snigdhas
Copy link
Contributor Author

Talked to @wedamija - we'll turn off the ff for writes and run the post-deploy migration to delete rows before running this migration with SafeRunSQL.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 13, 2025

This PR has a migration; here is the generated SQL for src/sentry/migrations/0927_make_open_period_range_boundary_inclusive.py

for 0927_make_open_period_range_boundary_inclusive in sentry

--
-- Custom state/database change combination
--
                    ALTER TABLE "sentry_groupopenperiod" ADD CONSTRAINT "exclude_overlapping_date_start_end" EXCLUDE USING GIST ("group" WITH =, (TSTZRANGE("date_started", "date_ended", '[]')) WITH &&);
                    
--
-- Remove constraint exclude_overlapping_start_end from model groupopenperiod
--
ALTER TABLE "sentry_groupopenperiod" DROP CONSTRAINT "exclude_overlapping_start_end";

Copy link
Member

@wedamija wedamija left a comment

Choose a reason for hiding this comment

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

Btw, if you want to delete the table quickly, you can use a TRUNCATE on the table so that you don't have to wait.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 16, 2025

This PR has a migration; here is the generated SQL for src/sentry/migrations/0930_make_open_period_range_boundary_inclusive.py

for 0930_make_open_period_range_boundary_inclusive in sentry

--
-- Custom state/database change combination
--
                    ALTER TABLE "sentry_groupopenperiod" ADD CONSTRAINT "exclude_overlapping_date_start_end" EXCLUDE USING GIST ("group_id" WITH =, (TSTZRANGE("date_started", "date_ended", '[]')) WITH &&);
                    
--
-- Remove constraint exclude_overlapping_start_end from model groupopenperiod
--
ALTER TABLE "sentry_groupopenperiod" DROP CONSTRAINT "exclude_overlapping_start_end";

name="exclude_overlapping_date_start_end",
expressions=[
(models.F("group"), RangeOperators.EQUAL),
("group", "="),
Copy link
Member

Choose a reason for hiding this comment

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

Why do we change this to be the string? Can't you just keep it as modes.F(...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm that was something I had changed when the migration check kept failing, but it shouldn't be necessary

@snigdhas snigdhas merged commit 5b3ad90 into master Jun 18, 2025
64 checks passed
@snigdhas snigdhas deleted the snigdha/fix-boundary branch June 18, 2025 16:06
andrewshie-sentry pushed a commit that referenced this pull request Jun 19, 2025
Some issues have the same regression/resolution time in our db and the
range boundary today would reject those as valid start/end times for
open periods. Updating the boundary to be inclusive on both ends should
fix this and allow an open period to start at the same time that the
previous one ends.
@github-actions github-actions bot locked and limited conversation to collaborators Jul 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants