-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
UPSTREAM: 51042: Allow passing request-timeout from NewRequest all the way down #13701
UPSTREAM: 51042: Allow passing request-timeout from NewRequest all the way down #13701
Conversation
@@ -222,9 +222,11 @@ func (c *RESTClient) Verb(verb string) *Request { | |||
backoff := c.createBackoffMgr() | |||
|
|||
if c.Client == nil { | |||
return NewRequest(nil, verb, c.base, c.versionedAPIPath, c.contentConfig, c.serializers, backoff, c.Throttle) | |||
return NewRequest(nil, verb, c.base, c.versionedAPIPath, c.contentConfig, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timeout should be a helper on Request, vs adding something to the constructor. There are lots of reasons to set timeouts per request type.
@smarterclayton Timeout
is already a separate method on Request
. I was thinking rather that we'll copy the value passed through client, and if someone will actually use Timeout
that will override that this. What I had in mind is that I wanted to have a global solution that will just pass the timeout. It's reasonable to use Timeout
method here instead of modifying the constructor, though.
… On Sun, May 28, 2017 at 5:13 AM, OpenShift Bot ***@***.***> wrote:
Origin Action Required: Pull request cannot be automatically merged,
please rebase your branch from latest HEAD and push again
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13701 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p84uFA23egWDDxU5xRdtrmHZJ6KJks5r-TqlgaJpZM4M41Xk>
.
|
Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again |
/assign smarterclayton |
The uptream part is kubernetes/kubernetes#51042 |
@@ -992,7 +992,7 @@ func (c *allClient) verb(verb string, gvk unversioned.GroupVersionKind) (*restcl | |||
if err != nil { | |||
return nil, err | |||
} | |||
return restclient.NewRequest(c.client, verb, baseURL, versionedAPIPath, contentConfig, *serializers, c.backoff, c.config.RateLimiter), nil | |||
return restclient.NewRequest(c.client, verb, baseURL, versionedAPIPath, contentConfig, *serializers, c.backoff, c.config.RateLimiter, 0), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry that I'm just now getting to the point of chasing this down, but why not just call restclient.NewRequest().Timeout(0)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, it doesn't matter. What I care is that the --request-timeout
flag is being properly propagated to the request.
c9b538d
to
e62f1fc
Compare
I'll merge this once I get upstream approval. |
Automatic merge from submit-queue (batch tested with PRs 59276, 51042, 58973, 59377, 59472). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Allow passing request-timeout from NewRequest all the way down **What this PR does / why we need it**: Currently if you pass `--request-timeout` it's not passed all the way down to the actual request object. There's a separate field on the `Request` object that allows setting that timeout, but it's not taken from that flag. @smarterclayton @deads2k ptal, this is coming from openshift/origin#13701
Automatic merge from submit-queue (batch tested with PRs 59276, 51042, 58973, 59377, 59472). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Allow passing request-timeout from NewRequest all the way down **What this PR does / why we need it**: Currently if you pass `--request-timeout` it's not passed all the way down to the actual request object. There's a separate field on the `Request` object that allows setting that timeout, but it's not taken from that flag. @smarterclayton @deads2k ptal, this is coming from openshift/origin#13701 Kubernetes-commit: 1f6251444b7dad7f5d924acbfb366541f2a6fb99
e62f1fc
to
3bc4d69
Compare
This was already merged upstream so adding necessary labels. |
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: soltysh The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/retest |
Automatic merge from submit-queue (batch tested with PRs 18503, 18399, 13701, 18513, 18515). |
This was split from #13458.
@smarterclayton let's continue the discussion here.