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

fix(CapMan): Allocation Policies causing potentially timeout errors on ST #4403

Merged
merged 2 commits into from
Jun 27, 2023

Conversation

rahul-kumar-saini
Copy link
Contributor

Overview

  • Currently there is some strong correlation between the latest query errors and timeouts on Disney ST and Allocation Policy throttles on the instance
  • Disney ST max threads are 30 by default, the throttling sets the queries to 1 thread - a massive difference
  • I don't believe we have a huge load problem on ST so we can probably disable the policy enforcement altogether
  • Ideally this is done through Snuba Admin but since that does not yet exist for ST this is a good enough solution

@rahul-kumar-saini rahul-kumar-saini requested a review from a team as a code owner June 22, 2023 21:42
@codecov
Copy link

codecov bot commented Jun 22, 2023

Codecov Report

Patch coverage: 87.96% and project coverage change: -0.01 ⚠️

Comparison is base (265a7a5) 90.35% compared to head (9206923) 90.34%.

❗ Current head 9206923 differs from pull request most recent head d0a952f. Consider uploading reports for the commit d0a952f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4403      +/-   ##
==========================================
- Coverage   90.35%   90.34%   -0.01%     
==========================================
  Files         803      803              
  Lines       39416    39514      +98     
  Branches      245      245              
==========================================
+ Hits        35613    35698      +85     
- Misses       3771     3784      +13     
  Partials       32       32              
Impacted Files Coverage Δ
snuba/cli/consumer.py 0.00% <0.00%> (ø)
snuba/cli/devserver.py 0.00% <ø> (ø)
snuba/cli/dlq_consumer.py 0.00% <ø> (ø)
snuba/datasets/processors/outcomes_processor.py 94.28% <ø> (ø)
snuba/consumers/consumer_builder.py 65.11% <33.33%> (-0.76%) ⬇️
snuba/consumers/consumer.py 90.11% <50.00%> (-0.26%) ⬇️
snuba/consumers/dlq.py 86.02% <50.00%> (-2.62%) ⬇️
snuba/admin/auth.py 84.00% <78.26%> (-3.04%) ⬇️
snuba/datasets/processors/querylog_processor.py 94.44% <83.33%> (-0.71%) ⬇️
...uba/datasets/processors/search_issues_processor.py 95.40% <100.00%> (+1.14%) ⬆️
... and 10 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@evanh evanh left a comment

Choose a reason for hiding this comment

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

Does this setting need to be enabled in prod_settings as well?

Copy link
Member

@volokluev volokluev left a comment

Choose a reason for hiding this comment

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

This is a hack that is necessary because single tenant does not have snuba-admin deployed so we can't change the configs there. Please leave a comment explaining that this is a hack and at what point it should be removed.

@rahul-kumar-saini rahul-kumar-saini enabled auto-merge (squash) June 27, 2023 17:58
@rahul-kumar-saini rahul-kumar-saini merged commit a77b2f9 into master Jun 27, 2023
20 checks passed
@rahul-kumar-saini rahul-kumar-saini deleted the rahul/fix/disable-allocation-policies-on-st branch June 27, 2023 19:56
@getsentry-bot
Copy link
Contributor

PR reverted: 703042e

getsentry-bot added a commit that referenced this pull request Mar 12, 2024
…errors on ST (#4403)"

This reverts commit a77b2f9.

Co-authored-by: volokluev <3169433+volokluev@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants