Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Backoff retry mcp client #20114

Merged
merged 3 commits into from
Feb 17, 2020
Merged

Backoff retry mcp client #20114

merged 3 commits into from
Feb 17, 2020

Conversation

hzxuzhonghu
Copy link
Member

@hzxuzhonghu hzxuzhonghu commented Jan 13, 2020 •
edited
Loading

EDIT: Make MCP client use backoff retry connect

@hzxuzhonghu hzxuzhonghu requested a review from a team January 13, 2020 02:42
@hzxuzhonghu hzxuzhonghu requested a review from a team as a code owner January 13, 2020 02:42
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jan 13, 2020
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 13, 2020
@hzxuzhonghu
Copy link
Member Author

TestMirroring flake

/test integ-pilot-k8s-tests_istio

ayj
ayj reviewed Jan 14, 2020
View reviewed changes
Copy link
Contributor

@ayj ayj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this fixing a specific issue?

pilot/pkg/serviceregistry/synthetic/serviceentry/syntheticserviceentrycontroller.go Outdated Show resolved Hide resolved
@hzxuzhonghu
Copy link
Member Author

Is this fixing a specific issue?

I am fixing a high latency issue in big scale when configs update qps is high.

@istio-testing istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 14, 2020
@hzxuzhonghu hzxuzhonghu force-pushed the mcp-sink branch 3 times, most recently from ebf5f47 to c110054 Compare January 16, 2020 03:08
@hzxuzhonghu
Copy link
Member Author

@ayj @howardjohn This is ready now.

@hzxuzhonghu
Copy link
Member Author

ping @howardjohn @ayj i have rebased many times

ayj
ayj reviewed Jan 23, 2020
View reviewed changes
pkg/mcp/sink/client_sink.go Outdated Show resolved Hide resolved
ayj
ayj reviewed Jan 23, 2020
View reviewed changes
pkg/mcp/sink/client_sink.go Outdated Show resolved Hide resolved
@hzxuzhonghu hzxuzhonghu requested a review from a team as a code owner February 7, 2020 08:03
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 7, 2020
@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 13, 2020
@hzxuzhonghu hzxuzhonghu changed the title Asynchronously handle mcp response Backoff retry mcp client Feb 13, 2020
@hzxuzhonghu
Copy link
Member Author

/retest

ayj
ayj reviewed Feb 14, 2020
View reviewed changes
pkg/mcp/sink/client_sink.go
// connect w/retry
for {
select {
case <-ctx.Done():
return
case <-time.After(retryDelay):
case <-t.C:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wouldn't hurt to call t.Stop() here when the context is done. The ticker's finalizer calls Stop but that could happen arbitrarily in the future.

@ayj ayj added the do-not-merge/hold Block automatic merging of a PR. label Feb 14, 2020
ayj
ayj approved these changes Feb 14, 2020
View reviewed changes
Copy link
Contributor

@ayj ayj left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I've added a /hold to give you an opportunity to address/comment on the remaining comment. Please feel free to remove it when you're done.

@hzxuzhonghu hzxuzhonghu removed the do-not-merge/hold Block automatic merging of a PR. label Feb 17, 2020
@istio-testing
Copy link
Collaborator

istio-testing commented Feb 17, 2020 •
edited
Loading

@hzxuzhonghu: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pilot-multicluster-e2e_istio b0ca5f390ae53e865459c04ca4917887d70970b5 link /test pilot-multicluster-e2e_istio

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.

@istio-testing istio-testing merged commit c50c8a4 into istio:master Feb 17, 2020
@hzxuzhonghu hzxuzhonghu deleted the mcp-sink branch February 17, 2020 07:43
sdake pushed a commit to sdake/istio that referenced this pull request Feb 21, 2020
@hzxuzhonghu
Backoff retry mcp client (istio#20114)
d56b5de 
* use backoff to reconnect to mcp server

* fix race

* fix ayj's comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ayj ayj ayj approved these changes

Assignees
No one assigned
Labels
area/perf and scalability cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hzxuzhonghu @istio-testing @ayj @googlebot