chore(hybrid-cloud): ORM decorators updated. #40022
Conversation
Models now have 3 states: control silo only, control silo with replication, and region silo only. Update our decorators to reflect that.
github-actions
bot
added
the
Scope: Backend
|
|
| @@ -224,7 +224,7 @@ class Meta: | |||
| db_table = "sentry_incidentsnapshot" | |||
|
|
|||
|
|
|||
| @control_silo_model | |||
| @control_silo_with_replication_model | |||
There was a problem hiding this comment.
Why does this need replication?
|
|
||
|
|
||
| @control_silo_model | ||
| @control_silo_with_replication_model |
There was a problem hiding this comment.
This should be region only. 🤔
|
|
||
|
|
||
| @control_silo_model | ||
| @control_silo_with_replication_model |
There was a problem hiding this comment.
This should be region only too.
| @@ -22,7 +22,7 @@ class Meta: | |||
| db_table = "sentry_relayusage" | |||
|
|
|||
|
|
|||
| @control_silo_model | |||
| @control_silo_with_replication_model | |||
| @@ -15,7 +15,7 @@ | |||
| from typing import Mapping, Type | |||
|
|
|||
|
|
|||
| @control_silo_model | |||
| @control_silo_with_replication_model | |||
There was a problem hiding this comment.
Yup. To be clear, this PR's sole focus is getting the orm decorators 1. renamed and 2. adding the new decorator focused on the replicated vs non replicated case.
I am not, focused on getting the decorators in the correct state. That would be this story: https://getsentry.atlassian.net/browse/HC-243
There was a problem hiding this comment.
Also, given that this is a decorator rename, in order to support https://github.com/getsentry/getsentry/pull/8577 can I, merge these two within short time when both are green and good, or is it required that I only merge this PR in with a backwards compatible naming, and merge an additional PR removing that rename after the getsentry pr?
There was a problem hiding this comment.
Conceptually, looks good to me! I'm not sure about every individual model but we can always change those as we go.
One consideration: we were tentatively committed to letting scripts/silo/ be a source of truth and to change model decorators only by updating and rerunning those scripts. I'm 100% okay with abandoning that approach if it isn't worthwhile, but we might want to drop a comment into those scripts, maybe a pointer to this PR, or just delete them entirely if we're feeling bold.
| all_silo_model = ModelSiloLimit(*SiloMode) | ||
| control_silo_with_replication_model = ModelSiloLimit(SiloMode.CONTROL, read_only=SiloMode.REGION) | ||
| control_silo_only_model = ModelSiloLimit(SiloMode.CONTROL) | ||
| region_silo_only_model = ModelSiloLimit(SiloMode.REGION) |
There was a problem hiding this comment.
If these global decorator aliases become more trouble than they're worth, I wouldn't be opposed to ditching them and just referring to ModelSiloLimit directly, as in
@ModelSiloLimit(SiloMode.CONTROL, read_only=SiloMode.REGION)
class TimeSeriesSnapshot(Model):I'm not going to advocate for that yet because "control silo with replication model" is a useful name that conveys information. But, if find ourselves adding more cases with increasingly verbose names, I wouldn't want to see it sprawl into an unmanageable menagerie of decorators. In that case, I'd think it's important to let the init parameters tell the whole story and refactor until they make sense.
There was a problem hiding this comment.
That's fair. Hopefully this is the final story for our data split, but if it does grow any more complex, I agree that we can just let the parameterization speak for itself.
Updating model decorator names reflecting newest hybrid cloud requirements.
As per the most recent discussion wrt hybrid cloud, models are now one of 3 possibilities:
Creating these three decorators and giving them updated names to reflect the new strategy. NOTE the decorators are still no-ops in production, and are likely to change as me and @RyanSkonnord move forward with locking these down.
Also, removed
all_silo_modelas it seems that no model will be allowed to exist in all silo modes.