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

Total Worker Parallelism in the Querier #2456

Merged
merged 48 commits into from
Apr 24, 2020
Merged

Total Worker Parallelism in the Querier #2456

merged 48 commits into from
Apr 24, 2020

Conversation

joe-elliott
Copy link
Contributor

What this PR does:

  • Adds a parameter -querier.worker-total-parallelism that controls a total number of concurrent requests allowed spread across workers.
  • Adds workerFrontendManager to help encapsulate functionality related to maintaining a set of concurrent workers per frontend.
  • Related tests/docs
  • Extends existing frontend tests to cover both the existing and new parallelism modes

Which issue(s) this PR fixes:
Fixes #1883

Checklist

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

@joe-elliott
Added param
50fb3c5 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
First pass new structure
1efc6d4 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Split out manager into its own code
3cda9ed 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
created interface to help with testing
662e364 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Cleaned up notes. Addressed concurrency issues
bae5fdf 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Added parallelism reset
cecf9b2 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Added total parallelism support
55b0d21 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Mirrored all tests for total parallelism
c0373c5 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Added clarity
b7f1858 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Removed upstream interface
818bb94 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Added first worker test
6c5e68b 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Added concurrency test
dcf2e45 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
TestTable for Concurrency
2e75c3f 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Added series of test cases
9c064cf 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Added test for failed receive. Fixed graceful quit shutdown
203ee30 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Added tests for number of calls made to the handler
67119bc 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Added test for cancelling service context cancelling processes
c2cc86f 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Added changelog entry and updated docs
9f2b58b 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
lint
992a3e2 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
lint part deux
9e504c3 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott joe-elliott changed the title Querier parallelism Total Worker Parallelism in the Querier Apr 14, 2020
@joe-elliott
make doc
9593d25 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Added resetParallelism tests and fixed bug
0fb4098 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Added stop test to resetParallelism
248ec80 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Added test names
ebd667f 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Cleaned up resetParallelism
670e157 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Added DNS Watch tests
b77ba05 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
lint
19cbec9 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Removed racey dns watcher tests
819b207 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Copy link
Contributor Author

joe-elliott commented Apr 16, 2020 •
edited
Loading

I believe all concerns have been addressed. I did prefer to pass the FrontendClient into the manager for reasons detailed above.

Please let me know if any other changes are desired!

@tomwilkie

@joe-elliott
Removed outdated comment
be78bab 
Signed-off-by: Joe Elliott <[email protected]>
tomwilkie
tomwilkie suggested changes Apr 17, 2020
View reviewed changes
Copy link
Contributor

@tomwilkie tomwilkie left a comment

Choose a reason for hiding this comment

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

Just nits left now. Thanks Joe!

@joe-elliott joe-elliott requested a review from tomwilkie April 17, 2020 17:37
@joe-elliott
Removed WithBlock() option
ac2fc0f 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott joe-elliott requested a review from tomwilkie April 19, 2020 13:51
@jtlisi jtlisi self-requested a review April 20, 2020 15:34
@tomwilkie tomwilkie requested review from pracucci and gouthamve April 21, 2020 12:35
jtlisi
jtlisi reviewed Apr 21, 2020
View reviewed changes
Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

One question. Otherwise this LGTM

@joe-elliott joe-elliott requested a review from jtlisi April 22, 2020 01:23
@tomwilkie tomwilkie merged commit 75c5eb5 into cortexproject:master Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomwilkie tomwilkie tomwilkie approved these changes

@pracucci pracucci Awaiting requested review from pracucci

@gouthamve gouthamve Awaiting requested review from gouthamve

@jtlisi jtlisi Awaiting requested review from jtlisi

Assignees
No one assigned
Labels
size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

querier.worker-parallelism should be derivable
3 participants
@joe-elliott @tomwilkie @jtlisi