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

feat(processing) Move redis operations in reprocessing to servicewrapper #66168

Merged
merged 4 commits into from Mar 5, 2024

Conversation

markstory
Copy link
Member

We need to shift the reprocessing workload to a new redis cluster without downtime. In order to do that I would like to use our Service wrappers and ServiceDelegator to dual write and then shift reads via runtime options.

Before I can move load with ServiceDelegator the existing logic needs to be moved into a service wrapper.

We need to shift the reprocessing workload to a new redis cluster
without downtime. In order to do that I would like to use our Service
wrappers and ServiceDelegator to dual write and then shift reads via
runtime options.

Before I can move load with ServiceDelegator the existing logic needs to
be moved into a service wrapper.
@markstory markstory requested a review from anonrig March 1, 2024 21:34
@markstory markstory requested a review from a team as a code owner March 1, 2024 21:34
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 1, 2024
@markstory markstory changed the title Chore reprocessing redis feat(processing) Move redis operations in reprocessing to servicewrapper Mar 1, 2024
import redis
from django.conf import settings

from sentry.utils import json
Copy link
Member

Choose a reason for hiding this comment

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

I see that you're only using dumps, explicit importing this would be better:

Suggested change
from sentry.utils import json
from sentry.utils.json import dumps

src/sentry/eventstore/reprocessing/redis.py Outdated Show resolved Hide resolved
for primary_hash in old_primary_hashes:
key = _get_old_primary_hash_subset_key(project_id, group_id, primary_hash)
event_count += client.llen(key)
if options.get(use_store_option):
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment to here?

src/sentry/eventstore/reprocessing/redis.py Outdated Show resolved Hide resolved
src/sentry/eventstore/reprocessing/redis.py Show resolved Hide resolved
Copy link

codecov bot commented Mar 1, 2024

Codecov Report

Attention: Patch coverage is 99.11504% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 84.26%. Comparing base (25a88ba) to head (7881f03).
Report is 27 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #66168      +/-   ##
==========================================
+ Coverage   84.24%   84.26%   +0.01%     
==========================================
  Files        5299     5304       +5     
  Lines      236744   236906     +162     
  Branches    40981    40993      +12     
==========================================
+ Hits       199442   199625     +183     
+ Misses      37083    37062      -21     
  Partials      219      219              
Files Coverage Δ
src/sentry/conf/server.py 90.06% <100.00%> (+0.03%) ⬆️
src/sentry/eventstore/reprocessing/__init__.py 100.00% <100.00%> (ø)
src/sentry/eventstore/reprocessing/redis.py 100.00% <100.00%> (ø)
src/sentry/options/defaults.py 100.00% <100.00%> (ø)
src/sentry/eventstore/reprocessing/base.py 94.73% <94.73%> (ø)

... and 35 files with indirect coverage changes

@markstory markstory requested a review from a team as a code owner March 4, 2024 15:57
Copy link
Member

@untitaker untitaker left a comment

Choose a reason for hiding this comment

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

i think it should be safe actually to skip the option entirely, and directly cut over to the service, for as long as the redis keys stay the same

@markstory markstory merged commit 127fc59 into master Mar 5, 2024
49 checks passed
@markstory markstory deleted the chore-reprocessing-redis branch March 5, 2024 17:19
aliu3ntry pushed a commit that referenced this pull request Mar 6, 2024
…per (#66168)

We need to shift the reprocessing workload to a new redis cluster
without downtime. In order to do that I would like to use our Service
wrappers and ServiceDelegator to dual write and then shift reads via
runtime options.

Before I can move load with ServiceDelegator the existing logic needs to
be moved into a service wrapper.
@github-actions github-actions bot locked and limited conversation to collaborators Mar 21, 2024
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.

None yet

3 participants