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

Improve http connection reuse #3760

Merged
merged 4 commits into from
Jun 20, 2024

Conversation

bmteller
Copy link
Contributor

@bmteller bmteller commented Jun 7, 2024

What this PR does:
Fixes connection reuse for S3 range requests and increases the number of MaxIdleConnsPerHost for the S3 backend to 100 to match the azure settings. Also, assumes GCP has the same connection reuse problem for range requests and performs the same fix. Note: I have tested the fix for S3 range requests and verified it is a problem and the change fixes the problem but I have not verified that it is a problem for GCP or that it fixes the problem for GCP. The GCP HTTP google storage code does look like it effectively returns the body reader so it does look like GCP would have a similar issue.

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

Checklist

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

@CLAassistant
Copy link

CLAassistant commented Jun 7, 2024

CLA assistant check
All committers have signed the CLA.

bmteller added 3 commits June 7, 2024 18:55
If EOF is not read then the connection is not eligble for reuse.
Try to read an extra byte to ensure EOF is read.
The previous value was the MinIO default which is 16 per
host and 100 global.
If EOF is not read then the connection is not eligble for reuse.
Try to read an extra byte to ensure EOF is read.
@bmteller bmteller force-pushed the improve_http_connection_reuse branch from be7a0ca to 7d00c3d Compare June 7, 2024 17:55
Copy link
Member

@joe-elliott joe-elliott 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 working on this.

  • Do you think we should put something similar in Azure?
  • Can you add a changelog entry detailing the fix

@bmteller
Copy link
Contributor Author

I don't think azure has a similar problem unless it is in the azure library itself.

        if _, err := blobClient.DownloadBuffer(ctx, destBuffer, &blob.DownloadBufferOptions{
                Range: blob.HTTPRange{
                        Offset: offset,
                        Count:  size,
                },
                BlockSize:   blob.DefaultDownloadBlockSize,
                Concurrency: maxParallelism,
                RetryReaderOptionsPerBlock: blob.RetryReaderOptions{
                        MaxRetries: maxRetries,
                },

and this blobClient is from github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/blob.

@bmteller
Copy link
Contributor Author

what should it be classified in the changelog as? BUGFIX?

@joe-elliott
Copy link
Member

what should it be classified in the changelog as? BUGFIX?

Yeah, I think that's fair.

Signed-off-by: Joe Elliott <[email protected]>
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

I went ahead and made the changelog entry b/c of how valuable this fix is.

Thanks!

@joe-elliott joe-elliott merged commit a210d3d into grafana:main Jun 20, 2024
14 checks passed
@bmteller
Copy link
Contributor Author

thanks Joe!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

S3 Not Reusing Connections
3 participants