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?
There was a problem hiding this comment.
ya good catch! That would not be ideal. Storing the httpClient in the CloudFetchConfig now so its lifetime is much longer.
samikshya-db
left a comment
samikshya-db
left a comment
There was a problem hiding this comment.
Thanks for the great addition! LGTM ✅
(Once you fix the lint issues, we are ready to merge this PR)
|
Thanks @Multimo !! Merged 🚢 |
|
Thanks @samikshya-db ! |
|
@Multimo @samikshya-db Just following through some of these PR's, could the fix be also needed for python connector as well for the issue #307 ? |
|
Hello @mohammedkhu - can you create an issue in the py connector describing your use-case/ requirements in detail? Open to code contributions there too! Thanks for reaching out. |
3 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 👍