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

Allow to override default retry wait times for Client #66

Merged
merged 3 commits into from
Apr 13, 2017

Conversation

bak1an
Copy link
Contributor

@bak1an bak1an commented Apr 7, 2017

This resolves #64

Changes made here:

  • Changing retry wait time options to have time.Duration type instead of of int holding milliseconds.

  • Adding RetryWaitTime and RetryMaxWaitTime fields to Client along with chaining setters.

  • A condition to make sure Backoff always sleeps at least WaitTime duration. Before this change it could sleep less than this due to jitter we have there. Especially before the very first retry, the sleep duration was always somewhere between WaitTime / 2 and WaitTime.

  • Test to make sure overridden wait times are actually used.

@codecov-io
Copy link

codecov-io commented Apr 7, 2017

Codecov Report

Merging #66 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
+ Coverage   96.74%   96.78%   +0.03%     
==========================================
  Files          11       11              
  Lines         892      902      +10     
==========================================
+ Hits          863      873      +10     
  Misses         15       15              
  Partials       14       14
Impacted Files Coverage Δ
client.go 95.69% <100%> (+0.09%) ⬆️
default.go 100% <100%> (ø) ⬆️
retry.go 100% <100%> (ø) ⬆️
request.go 100% <100%> (ø) ⬆️

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 c45c7bc...aa6636d. Read the comment docs.

@jeevatkm jeevatkm self-assigned this Apr 10, 2017
Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

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

Thank you for effort and time. Implementation looks perfect.

I think we have to add info about these defaults in godoc or Readme.md. What do you think?

defaultWaitTime    = time.Duration(100) * time.Millisecond
defaultMaxWaitTime = time.Duration(2000) * time.Millisecond

@bak1an
Copy link
Contributor Author

bak1an commented Apr 10, 2017

@jeevatkm Makes sense.

I'll add some info on retries to readme.

@jeevatkm
Copy link
Member

Thank you @bak1an.

@jeevatkm
Copy link
Member

@bak1an Shall I go ahead and merge this PR or wait for your readme changes? Please let me know.

@bak1an
Copy link
Contributor Author

bak1an commented Apr 13, 2017

@jeevatkm

Sorry for the delay,

Added some retry usage doc and mentioned delay defaults in doc.

@jeevatkm
Copy link
Member

Thank you @bak1an, I appreciate it.

@jeevatkm jeevatkm merged commit 97effac into go-resty:master Apr 13, 2017
@jeevatkm jeevatkm modified the milestone: v0.12 Milestone May 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add support for WaitTime and MaxWaitTime for retries
3 participants