Skip to content

fix(idtoken): increase MaxIdleConnsPerHost to 100 in NewClient#1754

Merged
quartzmo merged 2 commits intogoogleapis:mainfrom
quartzmo:idtoken-maxidleconnsperhost
Nov 21, 2022
Merged

fix(idtoken): increase MaxIdleConnsPerHost to 100 in NewClient#1754
quartzmo merged 2 commits intogoogleapis:mainfrom
quartzmo:idtoken-maxidleconnsperhost

Conversation

@quartzmo
Copy link
Copy Markdown
Member

closes: #1744

@quartzmo quartzmo requested a review from a team November 18, 2022 23:38
Comment thread idtoken/idtoken.go
opts = append(opts, option.WithTokenSource(ts), internaloption.SkipDialSettingsValidation())
t, err := htransport.NewTransport(ctx, http.DefaultTransport, opts...)
httpTransport := http.DefaultTransport.(*http.Transport).Clone()
httpTransport.MaxIdleConnsPerHost = 100
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems like a large increase. The default when not set is 2 for comparison. I wonder if there is a different way we could enable users to do something like this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is the same default set in transport/http/dial.go. See some potentially relevant discussion here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@david-gang Do you have any opinion on hardcoding to 100 here, as opposed to flexible configuration or some other value?

Copy link
Copy Markdown

@david-gang david-gang Nov 21, 2022

Choose a reason for hiding this comment

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

@quartzmo 100 is fine for me. In the product i work on, we have 100 hardcoded for every http client.

@quartzmo quartzmo enabled auto-merge (squash) November 21, 2022 21:58
@quartzmo quartzmo merged commit 629e217 into googleapis:main Nov 21, 2022
@quartzmo quartzmo deleted the idtoken-maxidleconnsperhost branch November 21, 2022 23:10
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.

How to set a higher MaxIdleConnsPerHost for idtoken client

3 participants