UPSTREAM: 51042: Allow passing request-timeout from NewRequest all the way down by soltysh · Pull Request #13701 · openshift/origin

soltysh · 2017-04-10T14:09:49Z

This was split from #13458.

@smarterclayton let's continue the discussion here.

soltysh · 2017-04-10T14:11:59Z

vendor/k8s.io/kubernetes/pkg/client/restclient/client.go


 	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,


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.

smarterclayton · 2017-05-31T18:06:33Z

https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1868/testReport/junit/github/com_openshift_origin_test_cmd_request-timeout/test_cmd_timeout_sh_19__executing__oc_login__u_testuser__p_test___request_timeout_1ns__expecting_failure_and_text__Timeout_exceeded_while_awaiting_headers_/ weird

…

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> .

openshift-bot · 2017-07-01T12:23:16Z

Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again

deads2k · 2017-07-31T13:00:51Z

/assign smarterclayton
/unassign

soltysh · 2017-08-23T11:05:41Z

The uptream part is kubernetes/kubernetes#51042

deads2k · 2017-10-23T19:40:30Z

test/integration/etcd_storage_path_test.go

 		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


Sorry that I'm just now getting to the point of chasing this down, but why not just call restclient.NewRequest().Timeout(0)?

Here, it doesn't matter. What I care is that the --request-timeout flag is being properly propagated to the request.

soltysh · 2018-02-06T13:30:59Z

I'll merge this once I get upstream approval.

@smarterclayton

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

@smarterclayton

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

…e way down

soltysh · 2018-02-08T10:15:31Z

This was already merged upstream so adding necessary labels.

openshift-ci-robot · 2018-02-08T10:15:48Z

[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.

Details

Needs approval from an approver in each of these OWNERS Files:

test/integration/OWNERS
vendor/k8s.io/kubernetes/pkg/OWNERS
vendor/k8s.io/kubernetes/staging/src/k8s.io/client-go/OWNERS
~~vendor/k8s.io/kubernetes/test/OWNERS~~ [soltysh]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

soltysh · 2018-02-08T12:43:59Z

/retest

openshift-merge-robot · 2018-02-09T00:52:25Z

Automatic merge from submit-queue (batch tested with PRs 18503, 18399, 13701, 18513, 18515).

soltysh commented Apr 10, 2017

View reviewed changes

soltysh mentioned this pull request Apr 10, 2017

Bug 1433244 - allow imageimports to be long runing requests #13458

Merged

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2017

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 24, 2017

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 9, 2017

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 22, 2017

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 1, 2017

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 1, 2017

openshift-merge-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 24, 2017

openshift-merge-robot assigned deads2k and eparis Jul 25, 2017

openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 28, 2017

openshift-ci-robot assigned smarterclayton and unassigned deads2k Jul 31, 2017

soltysh mentioned this pull request Aug 21, 2017

Allow passing request-timeout from NewRequest all the way down kubernetes/kubernetes#51042

Merged

openshift-merge-robot added the vendor-update Touching vendor dir or related files label Oct 14, 2017

deads2k reviewed Oct 23, 2017

View reviewed changes

soltysh force-pushed the request_timeout branch from c9b538d to e62f1fc Compare February 6, 2018 13:28

openshift deleted a comment from openshift-merge-robot Feb 6, 2018

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 6, 2018

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2018

soltysh changed the title ~~UPSTREAM: pass request-timeout all the way down to actual request~~ UPSTREAM: 51042: Allow passing request-timeout from NewRequest all the way down Feb 6, 2018

soltysh added 2 commits February 8, 2018 11:01

UPSTREAM: 51042: Allow passing request-timeout from NewRequest all th…

b0d47b8

…e way down

NewRequest signature applied in origin code

3bc4d69

soltysh force-pushed the request_timeout branch from e62f1fc to 3bc4d69 Compare February 8, 2018 10:14

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 8, 2018

soltysh added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. labels Feb 8, 2018

openshift-merge-robot merged commit 0fd7db9 into openshift:master Feb 9, 2018

soltysh deleted the request_timeout branch February 13, 2018 12:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
soltysh:request_timeout

soltysh commented Apr 10, 2017

Uh oh!

soltysh Apr 10, 2017

Uh oh!

smarterclayton commented May 31, 2017 via email

Uh oh!

openshift-bot commented Jul 1, 2017

Uh oh!

deads2k commented Jul 31, 2017

Uh oh!

soltysh commented Aug 23, 2017

Uh oh!

deads2k Oct 23, 2017

Uh oh!

soltysh Feb 6, 2018

Uh oh!

soltysh commented Feb 6, 2018

Uh oh!

soltysh commented Feb 8, 2018

Uh oh!

openshift-ci-robot commented Feb 8, 2018

Uh oh!

soltysh commented Feb 8, 2018

Uh oh!

openshift-merge-robot commented Feb 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

soltysh commented Apr 10, 2017

Uh oh!

soltysh Apr 10, 2017

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented May 31, 2017 via email

Uh oh!

openshift-bot commented Jul 1, 2017

Uh oh!

deads2k commented Jul 31, 2017

Uh oh!

soltysh commented Aug 23, 2017

Uh oh!

deads2k Oct 23, 2017

Choose a reason for hiding this comment

Uh oh!

soltysh Feb 6, 2018

Choose a reason for hiding this comment

Uh oh!

soltysh commented Feb 6, 2018

Uh oh!

soltysh commented Feb 8, 2018

Uh oh!

openshift-ci-robot commented Feb 8, 2018

Uh oh!

soltysh commented Feb 8, 2018

Uh oh!

openshift-merge-robot commented Feb 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants