Skip to content
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

remove references to AlertGroup.is_archived and AlertGroup.unarchived_objects #2524

Merged
merged 9 commits into from
Jul 18, 2023

Conversation

joeyorlando
Copy link
Contributor

What this PR does

This is a follow up to #2502 which started to remove logic to "archiving" alert groups. This PR:

  • removes all references to AlertGroup.is_archived and marks the column as deprecated. We will remove it in the next release
  • removes the AlertGroup.unarchived_objects Manager
  • renames the AlertGroup.all_objects Manager to AlertGroup.objects

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • CHANGELOG.md updated (or pr:no changelog PR label added if not required)

and AlertGroup.unarchived_objects. Also,
rename AlertGroup.all_objects to AlertGroup.objects
@joeyorlando joeyorlando requested a review from a team July 13, 2023 20:33
models.Index(
fields=["channel_id", "resolved", "acknowledged", "silenced", "root_alert_group_id", "is_archived"]
),
models.Index(fields=["channel_id", "resolved", "acknowledged", "silenced", "root_alert_group_id"]),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

come to think of it. we should first remove all references to unarchived_objects, and then remove this in the same PR as dropping the is_archived column entirely

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay so it seems like I need to remove this column from the index in this PR:

File "/Users/joeyorlando/coding/grafana/oncall/engine/apps/alerts/models/__init__.py", line 2, in <module>
    from .alert_group import AlertGroup  # noqa: F401
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/joeyorlando/coding/grafana/oncall/engine/apps/alerts/models/alert_group.py", line 135, in <module>
    class AlertGroup(AlertGroupSlackRenderingMixin, EscalationSnapshotMixin, models.Model):
  File "/Users/joeyorlando/.pyenv/versions/3.11.3/envs/3.11.3-oncall/lib/python3.11/site-packages/django/db/models/base.py", line 320, in __new__
    new_class._prepare()
  File "/Users/joeyorlando/.pyenv/versions/3.11.3/envs/3.11.3-oncall/lib/python3.11/site-packages/django/db/models/base.py", line 372, in _prepare
    index.set_name_with_model(cls)
  File "/Users/joeyorlando/.pyenv/versions/3.11.3/envs/3.11.3-oncall/lib/python3.11/site-packages/django/db/models/indexes.py", line 151, in set_name_with_model
    column_names = [model._meta.get_field(field_name).column for field_name, order in self.fields_orders]
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/joeyorlando/.pyenv/versions/3.11.3/envs/3.11.3-oncall/lib/python3.11/site-packages/django/db/models/indexes.py", line 151, in <listcomp>
    column_names = [model._meta.get_field(field_name).column for field_name, order in self.fields_orders]
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/joeyorlando/.pyenv/versions/3.11.3/envs/3.11.3-oncall/lib/python3.11/site-packages/django/db/models/options.py", line 599, in get_field
    raise FieldDoesNotExist(
django.core.exceptions.FieldDoesNotExist: AlertGroup has no field named 'is_archived'. The app cache isn't ready yet, so if this is an auto-created related field, it won't be available yet.

Copy link
Contributor Author

@joeyorlando joeyorlando Jul 18, 2023

Choose a reason for hiding this comment

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

hmm maybe a better approach here would be to briefly have two indices. First we would add the following in a separate prerequisite PR:

indexes = [
++    models.Index(fields=["channel_id", "resolved", "acknowledged", "silenced", "root_alert_group_id"]),
      models.Index(fields=["channel_id", "resolved", "acknowledged", "silenced", "root_alert_group_id", "is_archived"]),      
]

then once that has been released, merge this PR which drops the old index referencing is_archived. I don't think there should be any noticeable performance impact by briefly having two similar indices? But I'm more concerned about the brief period between when we drop the old index and create the new one. This could result in degraded performance on reads to the alert groups table.

However, I tested the CREATE INDEX (without is_archived field) DDL statement on a prod clone and it only took 4 minutes.

@Konstantinov-Innokentii wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that it's a four minute window, I'm going to merge this as is.

@joeyorlando joeyorlando added the pr:no public docs Added to a PR that does not require public documentation updates label Jul 18, 2023
@joeyorlando joeyorlando enabled auto-merge July 18, 2023 10:00
@joeyorlando joeyorlando disabled auto-merge July 18, 2023 10:00
@joeyorlando joeyorlando added this pull request to the merge queue Jul 18, 2023
Merged via the queue into dev with commit 9cc74e5 Jul 18, 2023
@joeyorlando joeyorlando deleted the jorlando/remove-alert-group-is-archived-column branch July 18, 2023 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants