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

chore(storage): add GCSFuse client config [benchmarking] #8225

Merged
merged 7 commits into from
Jul 7, 2023

Conversation

BrennaEpp
Copy link
Contributor

No description provided.

@BrennaEpp BrennaEpp requested review from a team as code owners July 6, 2023 19:45
@product-auto-label product-auto-label bot added size: s Pull request size is small. api: storage Issues related to the Cloud Storage API. labels Jul 6, 2023
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

Two small things, overall lgtm!

storage/internal/benchmarks/client_pool.go Outdated Show resolved Hide resolved
storage/internal/benchmarks/main.go Outdated Show resolved Hide resolved
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Jul 6, 2023
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

if err == nil {
http2Trans.ReadIdleTimeout = time.Second * 31
if config.setGCSFuseOpts {
base = &http.Transport{
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to create another object, since we are cloning the transport object in line:190. We can just override the attributes. Like,
base.MaxConnsPerHost = 100
base.MaxIdleConnsPerHost = 100
base.TLSNextProto = make(map[string]func(string, *tls.Conn) http.RoundTripper)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather keep the exact initialization code as gcsFuse has. http.DefaultTransport configures several transport variables to default values, and I'm not sure manually initializing an http.Transport would have those configured the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay.

Copy link
Contributor

@raj-prince raj-prince left a comment

Choose a reason for hiding this comment

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

Functionality wise looks good, have added minor refactoring comments.

@BrennaEpp BrennaEpp requested a review from raj-prince July 7, 2023 05:31
@BrennaEpp BrennaEpp enabled auto-merge (squash) July 7, 2023 20:34
@BrennaEpp BrennaEpp merged commit e1f52db into googleapis:main Jul 7, 2023
@BrennaEpp BrennaEpp deleted the bench-gcsfuse branch July 7, 2023 21:53
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. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants