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

Performance: Add MakeClientWithTransport client override that allows the user to pass a custom http transport #520

Merged
merged 7 commits into from
May 3, 2023

Conversation

pbennett
Copy link
Contributor

The sdk previously only used the default http client and the default client, using the default transport has severe limitations for apps making multiple connections to the same host. This new method will allow users to pass a new Transport instance which has new values for things like MaxIdleConnsPerHost and MaxIdleConns.

…ass a custom http RoundTripper (transport)

The sdk previously only used the default http client and the default client, using the default transport has severe limitations for apps making multiple connections to the same host.
This new method will allow users to pass a new Transport instance which has new values for things like MaxIdleConnsPerHost and MaxIdleConns.
@@ -139,7 +154,7 @@ func (client *Client) submitFormRaw(ctx context.Context, path string, params int

// Supply the client token.
req.Header.Set(client.apiHeader, client.apiToken)
// Add the client headers.
// Add the client headers.π
Copy link
Contributor

Choose a reason for hiding this comment

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

pi?

Choose a reason for hiding this comment

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

no, π

Choose a reason for hiding this comment

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

I think repo standard is 22/7

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol - darn hotkeys w/ focus in wrong place. I'll fix. 😀

@barnjamin barnjamin changed the title Add MakeClientWithTransport client override that allows the user to pass a custom http transport Performance: Add MakeClientWithTransport client override that allows the user to pass a custom http transport Apr 28, 2023
Copy link

@bbroder-algo bbroder-algo left a comment

Choose a reason for hiding this comment

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

Looks like a good idea to me, would like to see some use of it in algodclientv2_test.go

@pbennett
Copy link
Contributor Author

There are probably better summaries but here's one I found quickly describing why this is so important:
https://www.loginradius.com/blog/engineering/tune-the-go-http-client-for-high-performance/

For backend services using multiple goroutines which make many requests to algod instances (which will all be the same url), this should allow for a massive speed improvement.

Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

I definitely disagree about adding a cucumber test for this. The transport interface is specific to the go SDK, so no need test the other SDKs.

@bbroder-algo could you elaborate about what you had in mind? At most I'd think a unit test in common_test.go would be sufficient.

Regenerate import for indexer
@bbroder-algo bbroder-algo dismissed their stale review May 3, 2023 01:55

team lamprey

@algochoi algochoi merged commit 8a9ce2d into algorand:develop May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants