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

More improvements in HTTP client #388

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

pondzix
Copy link
Contributor

@pondzix pondzix commented Jan 8, 2025

ref: PDP-1643 PDP-1625 PDP-1624

Continuation of 065a10a:

  • Use http.DefaultTransport

It's because transport provided by net/http has some sensible defaults. Before that change we would use "empty" http.Transport{} with "empty" defaults.

  • Set MaxIdleConnsPerHost equal to the MaxIdleConns

In Snowbridge we basically target single host, so it makes sens to set MaxIdleConnsPerHost == MaxIdleConns.
MaxIdleConns provided by the http.DefaultTransport is 100. Before that change MaxIdleConnsPerHost was 2 (http.DefaultMaxIdleConnsPerHost).

  • Drain and close response body in defer function

@pondzix pondzix force-pushed the more_http_client_pooling_improvements branch 2 times, most recently from 5596a7e to 78250dd Compare January 13, 2025 12:46
Copy link
Collaborator

@colmsnowplow colmsnowplow left a comment

Choose a reason for hiding this comment

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

One comment to double check an assumption. As long as we have confidence in that, LGTM!

Continuation of 065a10a:

* Use `http.DefaultTransport`

It's because transport provided by `net/http` has some sensible defaults. Before that change we would use "empty" `http.Transport` with "empty" defaults.

* Set `MaxIdleConnsPerHost` equal to the `MaxIdleConns`

In Snowbridge we basically target single host, so it makes sens to set `MaxIdleConnsPerHost` == `MaxIdleConns`.
`MaxIdleConns` provided by the `http.DefaultTransport` is 100. Before that change `MaxIdleConnsPerHost` was 2  (`http.DefaultMaxIdleConnsPerHost`).

* Drain and close response body in defer function
@pondzix pondzix force-pushed the more_http_client_pooling_improvements branch from 78250dd to caf3386 Compare January 15, 2025 14:22
@pondzix pondzix merged commit caf3386 into release/3.1.0 Jan 15, 2025
1 check passed
@pondzix pondzix deleted the more_http_client_pooling_improvements branch January 15, 2025 14:22
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.

2 participants