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.

Sign up for GitHub

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

Jump to bottom

querier: Remove max concurrency reference to query-frontend #3678

Merged
merged 2 commits into from
Dec 12, 2022
Merged

querier: Remove max concurrency reference to query-frontend #3678

merged 2 commits into from
Dec 12, 2022

Conversation

aknuds1
Copy link
Contributor

@aknuds1 aknuds1 commented Dec 8, 2022 •
edited
Loading

What this PR does

Remove reference to query-frontend for flag -querier.max-concurrent, since it's (probably) not used by the query-frontend any longer.

@pstibrany observed that most likely the -querier.max-concurrent flag is not in use by the query-frontend since #2598, but its help string still says it has to be set also for the query-frontend.

TODO:

  • Conclude as to whether -querier.max-concurrent is really not in use by the query-frontend any longer

Which issue(s) this PR fixes or relates to

Checklist

  • [na] Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@aknuds1 aknuds1 requested a review from pracucci December 8, 2022 12:37
@pstibrany
Copy link
Member

max-concurrent was used to control concurrency of the PromQL engine in the past. Now we set engine concurrency to be unlimited. We still use the flag to control number of connections from querier to query-scheduler or frontend. But query-frontend has no usage for that flag now.

@aknuds1 aknuds1 added the bug Something isn't working label Dec 8, 2022
@aknuds1
Copy link
Contributor Author

aknuds1 commented Dec 8, 2022

max-concurrent was used to control concurrency of the PromQL engine in the past. Now we set engine concurrency to be unlimited. We still use the flag to control number of connections from querier to query-scheduler or frontend. But query-frontend has no usage for that flag now.

@pstibrany does that mean we can safely conclude that this change makes sense, and I can move the PR out of draft mode?

@pstibrany
Copy link
Member

max-concurrent was used to control concurrency of the PromQL engine in the past. Now we set engine concurrency to be unlimited. We still use the flag to control number of connections from querier to query-scheduler or frontend. But query-frontend has no usage for that flag now.

@pstibrany does that mean we can safely conclude that this change makes sense, and I can move the PR out of draft mode?

I'm pretty sure that's the case, but would like someone to double-check that finding.

@aknuds1
Copy link
Contributor Author

aknuds1 commented Dec 8, 2022 •
edited
Loading

I'll move it out of draft mode then and wait for another review. Thanks!

@aknuds1 aknuds1 marked this pull request as ready for review December 8, 2022 12:40
@aknuds1 aknuds1 requested review from a team as code owners December 8, 2022 12:40
@aknuds1 aknuds1 changed the title WIP: querier: Remove max concurrency reference to query-frontend querier: Remove max concurrency reference to query-frontend Dec 8, 2022
@aknuds1 aknuds1 force-pushed the bugfix/remove-max-concurrency-qf-reference branch from bfa6fc6 to 566bb16 Compare December 8, 2022 12:44
@aknuds1 aknuds1 added the type/docs Improvements or additions to documentation label Dec 8, 2022
@aknuds1 aknuds1 force-pushed the bugfix/remove-max-concurrency-qf-reference branch from 566bb16 to cf9db11 Compare December 8, 2022 13:50
pracucci
pracucci approved these changes Dec 12, 2022
View reviewed changes
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

I agree it's not used anymore in the query-frontend. LGTM (modulo a couple of nits).

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/querier/engine/config.go Outdated Show resolved Hide resolved
aknuds1 and others added 2 commits December 12, 2022 09:46
@aknuds1
querier: Remove max concurrency reference to query-frontend
9854ff2 
Remove reference to query-frontend for flag -querier.max-concurrent,
since it's not used by the query-frontend any longer.

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1 @pracucci
Update pkg/querier/engine/config.go
7f2120c 
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1 aknuds1 force-pushed the bugfix/remove-max-concurrency-qf-reference branch from d644ce4 to 7f2120c Compare December 12, 2022 08:48
@aknuds1 aknuds1 enabled auto-merge (squash) December 12, 2022 08:49
@aknuds1 aknuds1 disabled auto-merge December 12, 2022 08:49
@aknuds1 aknuds1 enabled auto-merge (squash) December 12, 2022 08:49
@aknuds1 aknuds1 merged commit b5251ff into main Dec 12, 2022
@aknuds1 aknuds1 deleted the bugfix/remove-max-concurrency-qf-reference branch December 12, 2022 09:01
masonmei pushed a commit to udmire/mimir that referenced this pull request Dec 16, 2022
@aknuds1 @pracucci
querier: Remove max concurrency reference to query-frontend (grafana#…
843a024 
…3678)

* querier: Remove max concurrency reference to query-frontend

Remove reference to query-frontend for flag -querier.max-concurrent,
since it's not used by the query-frontend any longer.

Also improve the -querier.max-concurrent help text a bit.

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pracucci pracucci pracucci approved these changes

Assignees
No one assigned
Labels
bug Something isn't working component/querier type/docs Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aknuds1 @pstibrany @pracucci