Cloudfetch: Allow configuration of httpclient for cloudfetch #308
Conversation
Multimo
requested review from
deeksha-db,
gopalldb,
jackyhu-db,
jadewang-db,
jayantsing-db,
jprakash-db,
madhav-db,
samikshya-db,
shivam2680 and
vikrantpuppala
as code owners
|
Thanks for making the clean fix, @Multimo ! One minor comment on renaming. |
connector.go
Outdated
| } | ||
|
|
||
| // WithCloudFetchHTTPClient allows a custom http client to be used for cloud fetch. Default is http.DefaultClient. | ||
| func WithCloudFetchHTTPClient(httpClient *http.Client) ConnOption { |
There was a problem hiding this comment.
Can we rename this to withHTTPClient ?
There was a problem hiding this comment.
Also, can you work with WithTransport option in this file for the private network use-case?
There was a problem hiding this comment.
An alternative, until you merge the two http clients, could be to use have the WithTransport add to the CloudFetchConfig also. I can update to do this if you prefer.
There was a problem hiding this comment.
Yes that would be great! thank you.
There was a problem hiding this comment.
Hey @samikshya-db, thanks for getting back to me. I have swapped it so same the transport will be re-used between both http clients.Let me know if this is what you had in mind.
Multimo
force-pushed
the
multimo/allow-cloudfetch-client-configuration
branch
from
4929871 to
934d2b5
Compare
| httpClient := http.DefaultClient | ||
| if transport != nil { | ||
| httpClient = &http.Client{ | ||
| Transport: transport, |
There was a problem hiding this comment.
Does this mean a new HttpClient is created on every fetch request when a custom transport is provided? If yes, can we make that more optimal?
2 participants
issues
Hello, as per issue looking for ways to modify the transport layer of the httpclient that cloudfetch uses. Happy to go another way to solving this, this just seamed like the simplest. Thanks for your work on the driver, its been very useful 👍