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

Bound parallelism frontend #3342

Merged
merged 3 commits into from
Feb 18, 2021

Conversation

cyriltovena
Copy link
Contributor

@cyriltovena cyriltovena commented Feb 16, 2021

What this PR does / why we need it:

This PR ensure that the parallelism in the frontend is correctly bound per query. Currently we only do that at each level in each middleware which means they don't know each other. This translate into explosion of parallelism when we split by time and by shard.

Example:

a 1h query splitted by 15m and sharded by 16 would use 64 parallelism even though the max parallelism per query is 16.

I believe this means that we will need to increase parallelism now that we really do listen to the max_query_parallelism.

I suggest we default to 64, but that's for another review.

Special notes for your reviewer:

This will be backported to Cortex, but to avoid the review burden I want to first try it in Loki. I don't think cortex suffers from this since it doesn't have yet sharding.

cc @pracucci @pstibrany @gouthamve

Checklist

  • Tests updated

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@codecov-io
Copy link

Codecov Report

Merging #3342 (b543401) into master (76e713f) will increase coverage by 0.00%.
The diff coverage is 73.58%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3342   +/-   ##
=======================================
  Coverage   62.72%   62.72%           
=======================================
  Files         205      205           
  Lines       17310    17361   +51     
=======================================
+ Hits        10857    10890   +33     
- Misses       5478     5492   +14     
- Partials      975      979    +4     
Impacted Files Coverage Δ
pkg/querier/queryrange/limits.go 83.83% <72.54%> (-12.00%) ⬇️
pkg/querier/queryrange/roundtrip.go 70.14% <100.00%> (ø)
pkg/promtail/targets/file/filetarget.go 62.75% <0.00%> (-4.14%) ⬇️
pkg/querier/queryrange/downstreamer.go 97.64% <0.00%> (+2.35%) ⬆️

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

We still need to

  • decrease frontend queue sizes (jsonnet)
  • remove the parallelism bounds in the sharding middleware

@owen-d owen-d merged commit 9950046 into grafana:master Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants