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(deletes): add use_bulk_deletes runtime config #6560

Merged
merged 2 commits into from
Nov 12, 2024

Conversation

MeredithAnya
Copy link
Member

@MeredithAnya MeredithAnya commented Nov 11, 2024

Adding a temporary runtime config (use_bulk_deletes) to allow for delete queries to go through the bulk delete pipeline, until we are ready to fully move over to using it.

I choose a runtime config because I didn't want sentry to have to be aware of which implementation was used (bulk deletes or not).

We should only enable this when we have the consumers running in S4S

@MeredithAnya MeredithAnya requested a review from a team as a code owner November 11, 2024 23:46
Copy link
Member

@onkar onkar left a comment

Choose a reason for hiding this comment

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

LGTM overall, added a couple of questions.

request_parts.query["query"]["columns"],
request_parts.attribution_info,
)
if get_config("use_bulk_deletes", False):
Copy link
Member

Choose a reason for hiding this comment

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

The default is False but shouldn't we set use_bulk_delete to False using set_config() in this PR and flip it to True once we are ready to use bulk delete in s4s? Also, the note on

def get_config(
says that get_config() is supposed to be deprecated. Does it make sense to add a function in snuba/state that returns bool and use it here?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh didn't see that comment - I'll update

and I can set the default to be 0 and use get_int_config

@MeredithAnya MeredithAnya merged commit 0d81bb9 into master Nov 12, 2024
30 checks passed
@MeredithAnya MeredithAnya deleted the meredith/SNS-2903 branch November 12, 2024 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants