fix(common): handle expired policies in *RetryLoop
#12301
Conversation
The retry policy may be expired when it enters the retry loop. For example, time-based policies may suffer from poor scheduling luck.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #12301 +/- ##
==========================================
- Coverage 93.56% 93.56% -0.01%
==========================================
Files 2012 2012
Lines 177918 177945 +27
==========================================
+ Hits 166475 166490 +15
- Misses 11443 11455 +12
☔ View full report in Codecov by Sentry. |
| @@ -61,41 +62,35 @@ template < | |||
| typename std::enable_if<google::cloud::internal::is_invocable< | |||
| Functor, RestContext&, Request const&>::value, | |||
| int>::type = 0> | |||
| auto RestRetryLoopImpl(std::unique_ptr<RetryPolicy> retry_policy, | |||
| std::unique_ptr<BackoffPolicy> backoff_policy, | |||
| auto RestRetryLoopImpl(RetryPolicy& retry_policy, BackoffPolicy& backoff_policy, | |||
There was a problem hiding this comment.
If we are going to make this change, we should do it in RetryLoopImpl too.
| // The last error cannot be retried, but it is not because the retry | ||
| // policy is exhausted. We call these "permanent errors", and they | ||
| // get a special message. | ||
| char const* prefix = !retry_policy.IsExhausted() | ||
| ? "Permanent error in" | ||
| : "Retry policy exhausted in"; |
There was a problem hiding this comment.
| // The last error cannot be retried, but it is not because the retry | |
| // policy is exhausted. We call these "permanent errors", and they | |
| // get a special message. | |
| char const* prefix = !retry_policy.IsExhausted() | |
| ? "Permanent error in" | |
| : "Retry policy exhausted in"; | |
| // Distinguish between a permanent error and too many transient errors. | |
| char const* prefix = retry_policy.IsExhausted() | |
| ? "Retry policy exhausted in" | |
| : "Permanent error in"; |
There was a problem hiding this comment.
Nit nit: It looks like RetryLoopError() could well handle the " in " part itself.
There was a problem hiding this comment.
Remove the negation(s). I am going to leave the in for a future PR.
| char const* prefix = !retry_policy.IsExhausted() | ||
| ? "Permanent error in" | ||
| : "Retry policy exhausted in"; | ||
| return internal::RetryLoopError(prefix, location, last_status); |
There was a problem hiding this comment.
observation: in the (rare) exhausted before start case, the error message is a bit redundant:
Retry policy exhausted in <location>: Retry policy exhausted before first request attempt
I am fine with this.
There was a problem hiding this comment.
I think I will open a separate thread to shave this baby yak cleanup the error messages from retry loops.
The retry policy may be expired when it enters the retry loop. For example, time-based policies may suffer from poor scheduling luck.
Part of the work for #12294
This change is