-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
chore(aci): make DetectorGroup unique on Group #102928
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/0095_unique_detectorgroup_group.py
Show resolved
Hide resolved
|
This PR has a migration; here is the generated SQL for for --
-- Alter field group on detectorgroup
--
SET CONSTRAINTS "workflow_engine_dete_group_id_9ff57c8f_fk_sentry_gr" IMMEDIATE; ALTER TABLE "workflow_engine_detectorgroup" DROP CONSTRAINT "workflow_engine_dete_group_id_9ff57c8f_fk_sentry_gr";
DROP INDEX CONCURRENTLY IF EXISTS "workflow_engine_detectorgroup_group_id_9ff57c8f";
CREATE UNIQUE INDEX CONCURRENTLY "workflow_engine_detectorgroup_group_id_9ff57c8f_uniq" ON "workflow_engine_detectorgroup" ("group_id");
ALTER TABLE "workflow_engine_detectorgroup" ADD CONSTRAINT "workflow_engine_detectorgroup_group_id_9ff57c8f_uniq" UNIQUE USING INDEX "workflow_engine_detectorgroup_group_id_9ff57c8f_uniq";
ALTER TABLE "workflow_engine_detectorgroup" ADD CONSTRAINT "workflow_engine_dete_group_id_9ff57c8f_fk_sentry_gr" FOREIGN KEY ("group_id") REFERENCES "sentry_groupedmessage" ("id") DEFERRABLE INITIALLY DEFERRED NOT VALID;
ALTER TABLE "workflow_engine_detectorgroup" VALIDATE CONSTRAINT "workflow_engine_dete_group_id_9ff57c8f_fk_sentry_gr";
--
-- Alter unique_together for detectorgroup (0 constraint(s))
--
ALTER TABLE "workflow_engine_detectorgroup" DROP CONSTRAINT "workflow_engine_detector_detector_id_group_id_02ffc513_uniq"; |
| migrations.AlterUniqueTogether( | ||
| name="detectorgroup", | ||
| unique_together=set(), | ||
| ), | ||
| migrations.AlterField( | ||
| model_name="detectorgroup", | ||
| name="group", | ||
| field=sentry.db.models.fields.foreignkey.FlexibleForeignKey( | ||
| on_delete=django.db.models.deletion.CASCADE, to="sentry.group", unique=True | ||
| ), | ||
| ), |
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.
We should switch this order around, otherwise we won't be covered by the unique index for a while and might introduce bad rows
| class Meta: | ||
| db_table = "workflow_engine_detectorgroup" | ||
| app_label = "workflow_engine" | ||
| unique_together = (("detector", "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.
I think we're safe enough without this index. If we search on detector_id, group_id it can just use the unique on group. The only case I can think of where this old index might have been helpful if we did where detector_id = x order by group_id, which doesn't seem like it'd happen too often
82c0867 to
b769a0c
Compare
src/sentry/workflow_engine/migrations/0095_unique_detectorgroup_group.py
Show resolved
Hide resolved
src/sentry/workflow_engine/migrations/0095_unique_detectorgroup_group.py
Show resolved
Hide resolved
saponifi3d
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.
lgtm, might be nice to have a second pass from @wedamija tho too.
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.
lgtm, this might end up being painful to create!
A group should only be associated with 1 detector. This is a huge table so making it post-deploy, but we still might fail to acquire the lock...