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

For v2 Release #249

Merged
merged 26 commits into from
Jun 19, 2019
Merged

For v2 Release #249

merged 26 commits into from
Jun 19, 2019

Conversation

jeevatkm
Copy link
Member

For insights refer to #235, #166, #167.

jeevatkm and others added 24 commits February 4, 2019 21:14
1) Add capabilities to handle Retry-After headers and similar info from
server

Motivation: some servers provide Retry-After header or similar info along with 429
or 503 status code, and it is often important to honor such information
on retries, i.e. simple expotential backoff is not optimal.
https://docs.microsoft.com/en-us/sharepoint/dev/general-development/how-to-avoid-getting-throttled-or-blocked-in-sharepoint-online

2) Add option NOT to retry even if operation returns an error (but retry
by default, if no retry conditions are set)

Motivation: error are already passed to condition callback in resty, but
Backoff() still retries the request if error is not nil. It implies excessive,
stillborn retries for non-retryble errors from underlying http client
(i.e. with RoundTripper from oauth2).

3) Remove error return value from condition callback

Motivation: this error is neither passed to caller, nor logged in any
way. It is cleaner to have "needRetry == true" than "needRetry == true
|| conditionErr != nil".

4) Does not use floating-point arithmetics for expotential backoff

Motivation: simplification & performance
Stop sleeping between attempts if request context was cancelled

Motivation: it may be important to finish all tasks on context cancel as soon as possible,
i.e. on graceful shutdown, before the process is killed.
@jeevatkm jeevatkm added this to the v2.0.0 Milestone milestone Jun 19, 2019
@jeevatkm jeevatkm self-assigned this Jun 19, 2019
@codecov
Copy link

codecov bot commented Jun 19, 2019

Codecov Report

Merging #249 into master will decrease coverage by 0.21%.
The diff coverage is 97.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #249      +/-   ##
==========================================
- Coverage   96.17%   95.95%   -0.22%     
==========================================
  Files           9        9              
  Lines        1123     1113      -10     
==========================================
- Hits         1080     1068      -12     
- Misses         23       24       +1     
- Partials       20       21       +1
Impacted Files Coverage Δ
redirect.go 94.11% <0%> (ø) ⬆️
response.go 96.42% <0%> (-3.58%) ⬇️
middleware.go 91.42% <100%> (+0.17%) ⬆️
trace.go 100% <100%> (ø)
request.go 100% <100%> (ø) ⬆️
resty.go 100% <100%> (ø)
util.go 92.8% <100%> (+0.49%) ⬆️
retry.go 97.33% <95.65%> (+1.77%) ⬆️
client.go 97.38% <98.87%> (+0.23%) ⬆️
... and 1 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 fa5875c...be9a314. Read the comment docs.

@jeevatkm jeevatkm merged commit e284be3 into master Jun 19, 2019
@jeevatkm jeevatkm deleted the v2 branch June 19, 2019 09:14
@jeevatkm jeevatkm restored the v2 branch June 19, 2019 09:28
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.

4 participants