change Backoff() algorithm #237
Conversation
|
I will glad to receive any comments! |
Codecov Report
@@ Coverage Diff @@
## v2 #237 +/- ##
==========================================
+ Coverage 95.81% 95.91% +0.09%
==========================================
Files 9 9
Lines 1076 1101 +25
==========================================
+ Hits 1031 1056 +25
Misses 24 24
Partials 21 21
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@neganovalexey Thank you for the PR.
I see a value in your explanations with motivation:
- Add capabilities to handle Retry-After headers and similar info from the server
- [Jeeva] Meaningful feature for the retry
- Add option NOT to retry even if the operation returns an error (but retry by default, if no retry conditions are set)
- [Jeeva] This is a kind of design choice, let's introduce this behavior and wait for user feedback to re-evaluate in the future on behavior.
- Remove error return value from condition callback
- [Jeeva] I like the simplification of return value to boolean, leaving error handling choice to the user
Curious to understand your reasoning -
- Does not use floating-point arithmetics for exponential backoff
- [Jeeva] Would you mind explaining it??
|
@neganovalexey Also, could you have a look at the test coverage? which edge case is missing from the unit test. |
Unfortunately, if request is always retried in the presence of non-nil error, passing this err to retry condition callback doesn't make much sense, as a user can't disable retry for particular kinds of errors (e.g. invalid token, etc.). |
done |
|
@jeevatkm , floating-point arithmetics is less precise than integer arithmetics. It doesn't matter for this code, the corresponding changes were done mainly for aesthetic reasons (it is painful to see avoidable floating-point operations for a person with some C background). |
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
@pborzenkov Its not always retried when error is non-nil. My understanding is - it happens only when retry count >0 and retry condition is not set. @neganovalexey could you please validate my understanding and respond to @pborzenkov? |
|
@jeevatkm I was referring to the original way the code was written. Merged patch definitely allows to stop retry for particular kinds of errors, which is good! Thanks! |
|
@pborzenkov Thank you 😄 |
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
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).
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".
Motivation: simplification & performance