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

ref(hybrid-cloud): Removes transaction silo routing from patch code #53080

Merged

Conversation

GabeVillalobos
Copy link
Member

With the inclusion of our recent changes to transaction validations in PR #52943 and the inclusion of 'using' in all of our transactions/helpers thanks to PRs #53027 and #53004, we should be able to safely remove the ability to specify a transaction without a using keyword arg.

Because this is now mandatory, it should also be safe to remove autorouting, which ended up being incorrect in many testing contexts anyway.

@GabeVillalobos GabeVillalobos requested a review from a team as a code owner July 18, 2023 16:44
@GabeVillalobos GabeVillalobos added the Trigger: Silo db Trigger tests with split databases that simulate hybrid cloud data separation in postgres label Jul 18, 2023
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 18, 2023
Copy link
Contributor

@corps corps left a comment

Choose a reason for hiding this comment

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

Love it!

@markstory
Copy link
Member

Do we have any transaction.atomic() calls to update in getsentry?

@GabeVillalobos
Copy link
Member Author

Do we have any transaction.atomic() calls to update in getsentry?

I believe @corps already patched most, if not all of them. Guess we'll find out 😁

@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Merging #53080 (c938090) into master (8a85e90) will increase coverage by 0.00%.
The diff coverage is 90.90%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #53080   +/-   ##
=======================================
  Coverage   79.49%   79.49%           
=======================================
  Files        4935     4935           
  Lines      207734   207737    +3     
  Branches    35452    35454    +2     
=======================================
+ Hits       165141   165149    +8     
+ Misses      37563    37558    -5     
  Partials     5030     5030           
Impacted Files Coverage Δ
...entry/silo/patches/silo_aware_transaction_patch.py 81.25% <77.77%> (-2.09%) ⬇️
src/sentry/models/organization.py 88.01% <100.00%> (+0.09%) ⬆️
src/sentry/services/hybrid_cloud/auth/impl.py 92.35% <100.00%> (ø)
src/sentry/silo/safety.py 88.00% <100.00%> (ø)
src/sentry/testutils/factories.py 92.87% <100.00%> (-0.03%) ⬇️
src/sentry/testutils/hybrid_cloud.py 88.70% <100.00%> (+0.18%) ⬆️
...c/sentry/web/frontend/unsubscribe_notifications.py 82.05% <100.00%> (ø)

... and 2 files with indirect coverage changes

@GabeVillalobos GabeVillalobos added the Trigger: Silo db Trigger tests with split databases that simulate hybrid cloud data separation in postgres label Jul 19, 2023
@GabeVillalobos GabeVillalobos force-pushed the remove-db-auto-selection-from-transaction-patch branch from f84393b to 9fa35f5 Compare July 19, 2023 00:18
@GabeVillalobos GabeVillalobos removed the Trigger: Silo db Trigger tests with split databases that simulate hybrid cloud data separation in postgres label Jul 19, 2023
@GabeVillalobos GabeVillalobos merged commit d38f5eb into master Jul 19, 2023
@GabeVillalobos GabeVillalobos deleted the remove-db-auto-selection-from-transaction-patch branch July 19, 2023 15:32
@asottile-sentry asottile-sentry added the Trigger: Revert Add to a merged PR to revert it (skips CI) label Jul 19, 2023
@getsentry-bot
Copy link
Contributor

PR reverted: 4705814

getsentry-bot added a commit that referenced this pull request Jul 19, 2023
…h code (#53080)"

This reverts commit d38f5eb.

Co-authored-by: asottile-sentry <103459774+asottile-sentry@users.noreply.github.com>
@asottile-sentry
Copy link
Member

reverting, got a failure in getsentry:

_____________ BackfillOrganizationMappingCustomerIds.test_backfill _____________
tests/getsentry/test_0243_backfill_organization_mapping_customer_ids.py:41: in setup_initial_state
    with unguarded_write():
/opt/hostedtoolcache/Python/3.8.16/x64/lib/python3.8/contextlib.py:240: in helper
    return _GeneratorContextManager(func, args, kwds)
/opt/hostedtoolcache/Python/3.8.16/x64/lib/python3.8/contextlib.py:83: in __init__
    self.gen = func(*args, **kwds)
E   TypeError: unguarded_write() missing 1 required positional argument: 'using'

@GabeVillalobos GabeVillalobos restored the remove-db-auto-selection-from-transaction-patch branch July 19, 2023 16:12
@GabeVillalobos GabeVillalobos deleted the remove-db-auto-selection-from-transaction-patch branch July 19, 2023 16:12
GabeVillalobos added a commit that referenced this pull request Jul 19, 2023
…using unguarded_write decorators (#53160)

With the inclusion of our recent changes to transaction validations in
PR #52943 and the inclusion of 'using' in all of our
transactions/helpers thanks to PRs #53027 and #53004, we should be able
to safely remove the ability to specify a transaction without a using
keyword arg.

Because this is now mandatory, it should also be safe to remove
autorouting, which ended up being incorrect in many testing contexts
anyway.

This is a resubmission of PR #53080 which was reverted due to broken
load-mocks and a single broken getsentry migration test.
@github-actions github-actions bot locked and limited conversation to collaborators Aug 4, 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 Trigger: Revert Add to a merged PR to revert it (skips CI)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants