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

Allow to disable http2 for GCS. #4942

Merged
merged 10 commits into from
Jan 3, 2022
Merged

Allow to disable http2 for GCS. #4942

merged 10 commits into from
Jan 3, 2022

Conversation

cyriltovena
Copy link
Contributor

@cyriltovena cyriltovena commented Dec 16, 2021 •
edited
Loading

This also tweaks our idle connection pooling across the board and reduce the insane parallelism when fetching chunks.

This is the result of an experiment I run in our cluster.

Signed-off-by: Cyril Tovena cyril.tovena@gmail.com

Checklist

  • Documentation added
  • Tests updated
  • Add an entry in the CHANGELOG.md about the changes.

@cyriltovena
Allow to disable http2 for GCS.
558a925 
This also tweaks our iddle connection pooling accross the board and reduce the insane parallelism when fetching chunks.

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@cyriltovena
update changelog
6256b14 
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
dannykopping
dannykopping reviewed Dec 16, 2021
View reviewed changes
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

Looks good; I think it'd be valuable to share some of the benchmarks we collected when testing this out - your PR description doesn't really describe the huge benefits this can have. Let's shout about it 🙂

pkg/storage/chunk/azure/blob_storage_client.go Outdated Show resolved Hide resolved
pkg/storage/chunk/gcp/gcs_object_client.go Outdated Show resolved Hide resolved
pkg/storage/chunk/gcp/instrumentation.go Show resolved Hide resolved
pkg/storage/chunk/util/parallel_chunk_fetch.go Outdated Show resolved Hide resolved
@cyriltovena
Copy link
Contributor Author

Looks good; I think it'd be valuable to share some of the benchmarks we collected when testing this out - your PR description doesn't really describe the huge benefits this can have. Let's shout about it 🙂

I was able to 2x my throughput but I'd prefer to run this in a real environment first to not oversell it.

cyriltovena and others added 2 commits December 16, 2021 13:47
@cyriltovena @dannykopping
Update pkg/storage/chunk/gcp/gcs_object_client.go
0b95a0e 
Co-authored-by: Danny Kopping <dannykopping@gmail.com>
@cyriltovena
update comment
1719363 
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
KMiller-Grafana
KMiller-Grafana suggested changes Dec 23, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
docs/sources/configuration/_index.md Outdated Show resolved Hide resolved
cyriltovena and others added 5 commits January 3, 2022 08:56
@cyriltovena @KMiller-Grafana
Update docs/sources/configuration/_index.md
7d0744b 
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
@cyriltovena @KMiller-Grafana
Update CHANGELOG.md
7434046 
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
@cyriltovena
Reduce MaxIdleConns to a more standard value.
d2126f7 
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@cyriltovena
Allow to configure max parallelism for fetching chunks
741658b 
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@cyriltovena
Merge remote-tracking branch 'upstream/main' into allow-disabling-http2
273167f 
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jan 3, 2022
@cyriltovena
Fixes tests with no default config
e099779 
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
dannykopping
dannykopping approved these changes Jan 3, 2022
View reviewed changes
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@cyriltovena cyriltovena enabled auto-merge (squash) January 3, 2022 11:25
@cyriltovena
Copy link
Contributor Author

@KMiller-Grafana I'll need approval for merging, Thanks !

KMiller-Grafana
KMiller-Grafana approved these changes Jan 3, 2022
View reviewed changes
Copy link
Contributor

@KMiller-Grafana KMiller-Grafana left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. LGTM.

@cyriltovena cyriltovena merged commit 03be8da into grafana:main Jan 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dannykopping dannykopping dannykopping approved these changes

@KMiller-Grafana KMiller-Grafana KMiller-Grafana approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@cyriltovena @dannykopping @KMiller-Grafana