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

chore(hybrid-cloud): Adds silo modes to most unmarked tasks #54086

Merged
merged 9 commits into from
Aug 8, 2023

Conversation

GabeVillalobos
Copy link
Member

No description provided.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Aug 3, 2023
@GabeVillalobos GabeVillalobos requested a review from a team August 3, 2023 01:52
@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Merging #54086 (7d65b42) into master (1ae45d5) will increase coverage by 0.00%.
Report is 20 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #54086   +/-   ##
=======================================
  Coverage   79.63%   79.63%           
=======================================
  Files        4982     4982           
  Lines      211093   211173   +80     
  Branches    35960    35962    +2     
=======================================
+ Hits       168094   168170   +76     
- Misses      37835    37837    +2     
- Partials     5164     5166    +2     
Files Changed Coverage Δ
src/sentry/snuba/tasks.py 96.42% <ø> (ø)
src/sentry/tasks/integrations/migrate_issues.py 88.88% <ø> (ø)
src/sentry/data_export/tasks.py 82.81% <100.00%> (+0.08%) ⬆️
...ynamic_sampling/tasks/boost_low_volume_projects.py 89.62% <100.00%> (+0.09%) ⬆️
...ic_sampling/tasks/boost_low_volume_transactions.py 93.97% <100.00%> (+0.02%) ⬆️
src/sentry/dynamic_sampling/tasks/collect_orgs.py 46.15% <100.00%> (+2.15%) ⬆️
.../sentry/dynamic_sampling/tasks/recalibrate_orgs.py 67.30% <100.00%> (+0.64%) ⬆️
...rc/sentry/dynamic_sampling/tasks/sliding_window.py 89.74% <100.00%> (+0.13%) ⬆️
...entry/dynamic_sampling/tasks/sliding_window_org.py 92.85% <100.00%> (+0.17%) ⬆️
src/sentry/incidents/tasks.py 86.36% <100.00%> (+0.12%) ⬆️
... and 58 more

... and 9 files with indirect coverage changes

@@ -92,6 +97,7 @@ def delete_unreferenced_blobs(blob_model, blob_index_model, blob_ids):
pass


# TODO(hybrid-cloud): Remove this once backfills are done?
Copy link
Member

Choose a reason for hiding this comment

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

I think the time for this to be removed is soon. I plan on going through avatar code this week so we can close of that pile of work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. I have a local commit that has started removing some of the fallback file lookup as part of the split db work but I can throw it on the backburner it if you're planning to tackle this since I think you have more context that I'm probably missing 😅 .

Copy link
Member

Choose a reason for hiding this comment

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

I had some time today to go through avatars again and put up #54118 to make writes use controlfile consistently but still have read support for region files (for single-tenant and self-hosted).

src/sentry/tasks/integrations/migrate_repo.py Outdated Show resolved Hide resolved
@Zylphrex
Copy link
Member

Zylphrex commented Aug 3, 2023

Should all new tasks explicitly specify which silo it should run in?

@GabeVillalobos
Copy link
Member Author

Should all new tasks explicitly specify which silo it should run in?

@Zylphrex Ideally yes, as it will allow us to validate silo-specific behavior a bit better in testing. @markstory probably has a more comprehensive answer as to when this is necessary though.

@GabeVillalobos GabeVillalobos force-pushed the add-silo-modes-to-instrumented-tasks branch from 2e86b8c to f85ae12 Compare August 4, 2023 16:16
@markstory
Copy link
Member

Should all new tasks explicitly specify which silo it should run in?

Ideally yes. However, it is most important for new control silo tasks, as if we have all of those annotated, we can infer which ones are region silo based. However, like Gabe said, having the annotations on all tasks helps us ensure that we're not crossing boundaries accidentally.

@GabeVillalobos GabeVillalobos merged commit 2d2c17b into master Aug 8, 2023
56 checks passed
@GabeVillalobos GabeVillalobos deleted the add-silo-modes-to-instrumented-tasks branch August 8, 2023 18:47
@github-actions github-actions bot locked and limited conversation to collaborators Aug 24, 2023
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