set timeout for bucket HEAD client #208
Conversation
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
cncf-cla: yes
|
@TerryHowe: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, TerryHowe The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/lgtm |
| func newHTTPClient() *http.Client { | ||
| // ensure sensible timeouts | ||
| // this client will be used to make HEAD calls | ||
| return &http.Client{ |
There was a problem hiding this comment.
TODO: transport and connection reuse tuning, given the large amount of calls to same-host?
There was a problem hiding this comment.
so you reuse the same http client?
There was a problem hiding this comment.
we do, and that will implicitly use http.DefaultTransport
http client transport can be tuned for connection idle + reuse
There was a problem hiding this comment.
client reuse may actually be suboptimal (locks on shared resources like cookies etc) vs just ensuring transport reuse and tuning that.
Most advice about client re-use seems to be either related to transport reuse (which can instead be accomplished by passing a transport when constructing a client) or maybe allocation (not a primary concern here).
k8s-ci-robot
removed
the
lgtm
|
rebased |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
k8s-ci-robot
added
the
lgtm
depends on #206
I haven't seen evidence that this is a problem yet, but it seems obviously problematic by inspection.
There is no default timeout on http.Client. We made a similar change for the listener.
It's possible to do more granular timeouts over e.g. dialing the connection, and other tuning, but this seems like the most important and simplest change.
/hold
Not sure if this counts as a bugfix ref: #181 as currently in practice we have very good response times and haven't seen this issue yet ... but it seems bound to happen.