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

create rc-processing-transaction #79200

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Conversation

kneeyo1
Copy link
Contributor

@kneeyo1 kneeyo1 commented Oct 16, 2024

This creates rc processing transactions redis cluster specifically for transactions. For now as part of the rollout, this only initializes the new eventstore.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Oct 16, 2024
@lynnagara
Copy link
Member

lynnagara commented Oct 16, 2024

there's a lot of places in code where you need to implement the dual writing, have you considered putting the dual writing logic in one place only inside the transaction_processing_store implementation, so that none of the callsites need to change when rolling this out?

@@ -524,7 +525,11 @@ def post_process_group(
)
return
with metrics.timer("tasks.post_process.delete_event_cache"):
event_processing_store.delete_by_key(cache_key)
Copy link
Member

Choose a reason for hiding this comment

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

do you also need to update line 520 to point at the transacations store?

@lynnagara
Copy link
Member

i think there are a few more places in the codebase where this is needed.. e.g.

unprocessed = event_processing_store.get(
cache_key_for_event({"project": event.project_id, "event_id": event.event_id}),
unprocessed=True,
)

can you check the whole codebase?

Comment on lines +573 to +576
if event_type == "transaction":
processing.transaction_processing_store.delete_by_key(cache_key)
else:
processing.event_processing_store.delete_by_key(cache_key)
Copy link
Member

@lynnagara lynnagara Oct 16, 2024

Choose a reason for hiding this comment

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

you have this if/else block twice in this file..

suggest to refactor to something like:

processing_store = transaction_processing_store if event_type == "transaction" else event_processing_store
processing_store.get(cache_key)
processing_store.store(data)
processing_store.delete_by_key(cache_key)

also, there needs to be a 3rd time, as line 509 also needs to go to either the event or transaction processing store

@kneeyo1
Copy link
Contributor Author

kneeyo1 commented Oct 17, 2024

i think there are a few more places in the codebase where this is needed.. e.g.

unprocessed = event_processing_store.get(
cache_key_for_event({"project": event.project_id, "event_id": event.event_id}),
unprocessed=True,
)

can you check the whole codebase?

that is from the _nodestore_save_many function which checks first to ensure the event type is not a transaction

if event.get_event_type() not in ("transaction", "generic") and job["groups"]:

The rest of the references in the codebase are test cases (and reprocessing, which I'm not sure if it needs to be updated) but I think they don't need to know about the split

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants