-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(aci): Make Workflow.when_condition_group unique #103768
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
src/sentry/workflow_engine/migrations/0103_workflow_when_condition_group_unique.py
Outdated
Show resolved
Hide resolved
wedamija
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.
Looks like we just need to rebase this. Just spreading info, you can use ./bin/update-migration to rebase this on top of the latest migration quickly
| # is a schema change, it's completely safe to run the operation after the code has deployed. | ||
| # Once deployed, run these manually via: https://develop.sentry.dev/database-migrations/#migration-deployment | ||
|
|
||
| is_post_deployment = False |
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 table has 2m rows. I always forget how long it takes to add indexes, but it might be safer to just do this as a post deploy migration to make sure we don't block deploys
src/sentry/workflow_engine/migrations/0103_workflow_when_condition_group_unique.py
Outdated
Show resolved
Hide resolved
991d43d to
22d31ea
Compare
|
This PR has a migration; here is the generated SQL for for --
-- Alter field when_condition_group on workflow
--
SET CONSTRAINTS "workflow_engine_work_when_condition_group_11d9ba05_fk_workflow_" IMMEDIATE; ALTER TABLE "workflow_engine_workflow" DROP CONSTRAINT "workflow_engine_work_when_condition_group_11d9ba05_fk_workflow_";
DROP INDEX CONCURRENTLY IF EXISTS "workflow_engine_workflow_when_condition_group_id_11d9ba05";
CREATE UNIQUE INDEX CONCURRENTLY "workflow_engine_workflow_when_condition_group_id_11d9ba05_uniq" ON "workflow_engine_workflow" ("when_condition_group_id");
ALTER TABLE "workflow_engine_workflow" ADD CONSTRAINT "workflow_engine_workflow_when_condition_group_id_11d9ba05_uniq" UNIQUE USING INDEX "workflow_engine_workflow_when_condition_group_id_11d9ba05_uniq";
ALTER TABLE "workflow_engine_workflow" ADD CONSTRAINT "workflow_engine_work_when_condition_group_11d9ba05_fk_workflow_" FOREIGN KEY ("when_condition_group_id") REFERENCES "workflow_engine_dataconditiongroup" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "workflow_engine_workflow" VALIDATE CONSTRAINT "workflow_engine_work_when_condition_group_11d9ba05_fk_workflow_"; |
Oof, this is going to drop the index first and we'll be uncovered until the unique recreates. I think we should try to structure this so that it creates the unique first, otherwise we might cause some db load if we make queries on this column |
We could do this by setting up the constraint like this: and generating the migration, then setting |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #103768 +/- ##
============================================
+ Coverage 66.03% 80.62% +14.58%
============================================
Files 9271 9279 +8
Lines 395607 396080 +473
Branches 25247 25247
============================================
+ Hits 261253 319343 +58090
+ Misses 133894 76277 -57617
Partials 460 460 |
|
This PR has a migration; here is the generated SQL for for --
-- Custom state/database change combination
--
CREATE UNIQUE INDEX CONCURRENTLY "workflow_engine_workflow_when_condition_group_id_11d9ba05_uniq" ON "workflow_engine_workflow" ("when_condition_group_id");
ALTER TABLE "workflow_engine_workflow" ADD CONSTRAINT "workflow_engine_workflow_when_condition_group_id_11d9ba05_uniq" UNIQUE USING INDEX "workflow_engine_workflow_when_condition_group_id_11d9ba05_uniq";
SET CONSTRAINTS "workflow_engine_work_when_condition_group_11d9ba05_fk_workflow_" IMMEDIATE; ALTER TABLE "workflow_engine_workflow" DROP CONSTRAINT "workflow_engine_work_when_condition_group_11d9ba05_fk_workflow_";
DROP INDEX CONCURRENTLY IF EXISTS "workflow_engine_workflow_when_condition_group_id_11d9ba05";
ALTER TABLE "workflow_engine_workflow" ADD CONSTRAINT "workflow_engine_work_when_condition_group_11d9ba05_fk_workflow_" FOREIGN KEY ("when_condition_group_id") REFERENCES "workflow_engine_dataconditiongroup" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "workflow_engine_workflow" VALIDATE CONSTRAINT "workflow_engine_work_when_condition_group_11d9ba05_fk_workflow_"; |
| # Required as the 'when' condition for the workflow, this evaluates states emitted from the detectors | ||
| when_condition_group = FlexibleForeignKey( | ||
| "workflow_engine.DataConditionGroup", null=True, blank=True | ||
| "workflow_engine.DataConditionGroup", null=True, blank=True, db_index=False |
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 was thinking that the final state could actually have unique=True here and not have the UniqueConstraint, just to use those as the db operations. But tbh it doesn't really matter which way we do it as long as both exist, so this lgtm
I was going to document this assumption, but it's roughly just as easy to require it and avoid potential weirdness down the road.