Add timeout to remote handler by bianpengyuan · Pull Request #9475 · istio/istio

bianpengyuan · 2018-10-23T05:52:03Z

douglas-reid · 2018-10-23T16:21:46Z

mixer/pkg/protobuf/yaml/dynamic/handler.go

non-blocking: should we have used a gogo annotation on this proto to automatically handle this conversion?

see: https://github.com/gogo/protobuf/blob/master/test/types/types.proto#L82

Thanks for the link! Created istio/api#673 and we can wait for that to be checked in then update it here. master branch is locked anyway.

douglas-reid · 2018-10-23T16:22:49Z

mixer/test/spybackend/nosession.go

we want this separation (between imports and istio-specific imports) still, right @mandarjog ?

Recovered to what is was. IDE auto formatted it.

codecov · 2018-10-23T22:59:53Z

Codecov Report

Merging #9475 into release-1.1 will increase coverage by 6%.
The diff coverage is 0%.

@@              Coverage Diff               @@
##           release-1.1   #9475      +/-   ##
==============================================
+ Coverage           63%     69%      +6%     
==============================================
  Files              559     442     -117     
  Lines            51528   41333   -10195     
==============================================
- Hits             32384   28176    -4208     
+ Misses           17285   11750    -5535     
+ Partials          1859    1407     -452

Impacted Files	Coverage Δ
mixer/pkg/protobuf/yaml/dynamic/handler.go	`0% <0%> (-73%)`	⬇️
mixer/adapter/inventory.gen.go	`0% <0%> (-100%)`	⬇️
mixer/pkg/protobuf/yaml/dynamic/encoder.go	`0% <0%> (-96%)`	⬇️
...ixer/pkg/protobuf/yaml/dynamic/valueTypeEncoder.go	`14% <0%> (-81%)`	⬇️
mixer/pkg/protobuf/yaml/dynamic/encoderUtil.go	`24% <0%> (-76%)`	⬇️
mixer/pkg/protobuf/yaml/dynamic/primitive.go	`38% <0%> (-60%)`	⬇️
mixer/pkg/protobuf/yaml/dynamic/builder.go	`44% <0%> (-55%)`	⬇️
mixer/adapter/rbac/rbac.go	`0% <0%> (-50%)`	⬇️
mixer/pkg/protobuf/yaml/resolver.go	`60% <0%> (-35%)`	⬇️
... and 211 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a363120...2523da6. Read the comment docs.

googlebot · 2018-11-15T18:18:05Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

stale · 2018-11-29T19:00:01Z

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

googlebot · 2018-12-01T01:33:39Z

CLAs look good, thanks!

bianpengyuan · 2018-12-03T22:58:58Z

/retest

bianpengyuan · 2018-12-04T01:39:58Z

/retest

bianpengyuan · 2018-12-04T05:37:53Z

/test e2e-dashboard
/test istio-pilot-e2e-envoyv2-v1alpha3

bianpengyuan · 2018-12-04T19:07:16Z

/test e2e-dashboard

istio-testing · 2018-12-04T19:14:57Z

@bianpengyuan: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/istio-integ-k8s-tests.sh	4285f7fb54ebf8d47f34b25c783139e9167e0b2f	link	`/test istio-integ-k8s-tests`
prow//e2e-simpleTests-minProfile.sh	b6cb5a0b34e2bda51e11db013732e8e148ed14df	link	`/test e2e-simpleTestsMinProfile`
prow/e2e-mixer-no_auth-mcp.sh	b6cb5a0b34e2bda51e11db013732e8e148ed14df	link	`/test e2e-mixer-no_auth-mcp`

Details

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. I understand the commands that are listed here.

bianpengyuan · 2018-12-04T21:40:32Z

ptal? thanks!

bianpengyuan · 2018-12-07T01:17:57Z

@douglas-reid all tests pass now, ptal? thanks!

kyessenov · 2018-12-07T01:21:34Z

mixer/pkg/protobuf/yaml/dynamic/handler_test.go

+	}
+	elapse := time.Since(start)
+	if elapse > 20*time.Millisecond {
+		t.Errorf("want elapse time less than 20 milliseconds, got %v", elapse)


This probably won't work in race test setting? Under race testing, an interrupt can be placed anywhere in the test. That would cause the elapsed time to exceed the limit for unrelated reasons.

Thanks for pointing this out! I guess I should give some headroom here to accommodate race test interruption? I raised it to 300ms The test adapter will sleep for 1 sec. As long as the elapse time is less 1 sec, timeout should be functioning.

bianpengyuan · 2018-12-11T18:01:54Z

@kyessenov @douglas-reid Can I get a lgtm? Thanks!

douglas-reid

/lgtm

istio-testing · 2018-12-11T18:58:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bianpengyuan, douglas-reid

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~mixer/OWNERS~~ [douglas-reid]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bianpengyuan requested a review from mandarjog October 23, 2018 05:52

googlebot added the cla: yes label Oct 23, 2018

istio-testing requested review from douglas-reid and geeknoid October 23, 2018 05:52

douglas-reid reviewed Oct 23, 2018

View reviewed changes

bianpengyuan mentioned this pull request Oct 23, 2018

Add stdduration annotation to timeout istio/api#673

Merged

bianpengyuan force-pushed the timeout branch from aabb813 to 2c9383f Compare October 23, 2018 23:01

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Oct 30, 2018

bianpengyuan force-pushed the timeout branch from 7229930 to b07519d Compare October 30, 2018 04:04

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Oct 30, 2018

bianpengyuan force-pushed the timeout branch 2 times, most recently from 1611472 to ca471ac Compare October 31, 2018 04:13

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Nov 10, 2018

bianpengyuan force-pushed the timeout branch from ca471ac to 4285f7f Compare November 15, 2018 18:17

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Nov 15, 2018

bianpengyuan changed the base branch from master to release-1.1 November 15, 2018 18:17

googlebot added cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. and removed cla: yes labels Nov 15, 2018

stale bot added the stale label Nov 29, 2018

bianpengyuan force-pushed the timeout branch from 4285f7f to 67ad51b Compare December 1, 2018 01:33

stale bot removed the stale label Dec 1, 2018

googlebot added cla: yes and removed cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. labels Dec 1, 2018

bianpengyuan force-pushed the timeout branch 2 times, most recently from b0446cf to 88781d1 Compare December 1, 2018 01:41

bianpengyuan force-pushed the timeout branch from 88781d1 to b6cb5a0 Compare December 3, 2018 23:11

timeout for remote handler

5af096b

bianpengyuan force-pushed the timeout branch from b6cb5a0 to 5af096b Compare December 4, 2018 20:42

kyessenov reviewed Dec 7, 2018

View reviewed changes

accommodate race test delay.

2523da6

douglas-reid approved these changes Dec 11, 2018

View reviewed changes

istio-testing assigned douglas-reid Dec 11, 2018

istio-testing added the lgtm label Dec 11, 2018

istio-testing added the approved label Dec 11, 2018

istio-testing merged commit 960fc43 into istio:release-1.1 Dec 11, 2018

Conversation

bianpengyuan commented Oct 23, 2018

Uh oh!

douglas-reid Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

bianpengyuan Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

douglas-reid Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

bianpengyuan Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

googlebot commented Nov 15, 2018

Uh oh!

stale bot commented Nov 29, 2018

Uh oh!

googlebot commented Dec 1, 2018

Uh oh!

bianpengyuan commented Dec 3, 2018

Uh oh!

bianpengyuan commented Dec 4, 2018

Uh oh!

bianpengyuan commented Dec 4, 2018

Uh oh!

bianpengyuan commented Dec 4, 2018

Uh oh!

istio-testing commented Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bianpengyuan commented Dec 4, 2018

Uh oh!

bianpengyuan commented Dec 7, 2018

Uh oh!

kyessenov Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

bianpengyuan Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

bianpengyuan commented Dec 11, 2018

Uh oh!

douglas-reid left a comment

Choose a reason for hiding this comment

Uh oh!

istio-testing commented Dec 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bianpengyuan Oct 23, 2018 •

edited

Loading

codecov bot commented Oct 23, 2018 •

edited

Loading

istio-testing commented Dec 4, 2018 •

edited

Loading