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.

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

Adds client retry. Fixes #959 #1119

Merged
merged 1 commit into from Feb 13, 2019
Merged

Adds client retry. Fixes #959 #1119

merged 1 commit into from Feb 13, 2019

Conversation

alexec
Copy link
Contributor

@alexec alexec commented Feb 13, 2019

  • Discus the correct configuration.

@codecov-io
Copy link

codecov-io commented Feb 13, 2019

Codecov Report

Merging #1119 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1119   +/-   ##
=======================================
  Coverage   37.24%   37.24%           
=======================================
  Files          53       53           
  Lines        7716     7716           
=======================================
  Hits         2874     2874           
+ Misses       4416     4413    -3     
- Partials      426      429    +3
Impacted Files Coverage Δ
server/server.go 45.67% <0%> (-0.73%) ⬇️
util/webhook/webhook.go 44.94% <0%> (+3.37%) ⬆️

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 70b0194...7f8a1ec. Read the comment docs.

Copy link
Collaborator

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

Did you need to run dep ensure to see if Gopkg.lock needs updating?

We should look into possibly improving the CLI to leverage this in the future.

@alexec
Copy link
Contributor Author

alexec commented Feb 13, 2019

I don't know if you need to run dep ensure. Maybe run it anyway.

@alexec
Copy link
Contributor Author

alexec commented Feb 13, 2019

Note to self on testing:

It takes up to 20s for a new pod to come up locally. So the 3s retry is not long enought to outlast this. Modify the timeout to be >20s (e.g. 30s).

Then, delete the repo-server:

kubectl delete $(kubectl get pods -o name|grep repo-server) && watch kubectl get pods

And hard refresh an app.

@alexec alexec merged commit 4283b8a into argoproj:master Feb 13, 2019
@alexec alexec deleted the grpc-retry branch February 13, 2019 23:32
jessesuen pushed a commit that referenced this pull request Feb 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants