-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
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.
@@ -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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, renamed to useGRPCBidiReads to match the experimental option
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.