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

feat(storage): Benchmark with experimental MRD. #11501

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cjc25
Copy link
Contributor

@cjc25 cjc25 commented Jan 24, 2025

Allow benchmarks to use the experimental MultiRangeDownloader for normal range reads.

Note that this does not use a single MultiRangeDownloader across multiple reads, which is supported but would require a more invasive change to the benchmark.

Allow benchmarks to use the experimental MultiRangeDownloader for normal
range reads.

Note that this does not use a single MultiRangeDownloader across
multiple reads, which is supported but would require a more invasive
change to the benchmark.
@cjc25 cjc25 requested review from a team as code owners January 24, 2025 15:42
@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Jan 24, 2025
@@ -247,6 +250,9 @@ func initializeGRPCClient(ctx context.Context, config clientConfig) (*storage.Cl
if config.readBufferSize != useDefault {
opts = append(opts, option.WithGRPCDialOption(grpc.WithReadBufferSize(config.readBufferSize)))
}
if config.useMultiRangeDownloader {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would only use bidi reads via storage.Reader, not actually use MultiRangeDownloader.

I think this is fine but maybe we should rename the opt; or just have Bidi reads be a transport option instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

useMultiRangeDownloaderForSingleRangeReads?

It gets a little unwieldy :) But I agree that users might be a little misled by the current name. I'm open to suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to Chris, I would go with having the option for bidi reads

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants