Repository navigation
Replies: 4 comments
ReviewA retry package in bedrock is defensible, but not for the same reasons as the circuit breaker, and only if it does more than a loop does. The two-step API carries over in part. Sitting inline in the caller's code carries over completely. The ticket and 1. Drop the freshness argumentIt doesn't apply to retry. The leading libraries are among the most active in the ecosystem:
There is a stale tail: jpillora/backoff v1.0.0 from 2019, lestrrat-go/backoff v2.0.8 from 2021 and Rican7/retry v0.3.1 from 2021. Nobody would point to it, though. Rewording won't save this argument the way it could in #595, so drop it. 2. Say exactly how existing retry libraries break the ethosThis is where the real case is. Each point below comes from a library's default path:
sethvargo/go-retry is the exception, and the one the request has to answer. It retries only errors marked with
The request shouldn't use panics as an argument. Unlike gobreaker, none of the four libraries calls 3. Make it more than a loop: put a retry budget in scopeThe circuit breaker earned a package because it holds shared state with invariants that are hard to keep under concurrency. A per-call retry loop holds none: it's an attempt counter, a delay and a wait, about 20 lines inline. Without shared state, a bedrock retry package is a helper teams can write themselves, and owning it is hard to justify. The shared state in retrying is a retry budget: a limit, per dependency, on how much of the traffic may be retries. Per-call limits don't stop a retry storm. They multiply across layers: 3 attempts at each of 3 layers is up to 27 attempts at the bottom. They also multiply across every caller during an outage. Prior art:
cenkalti/backoff, avast/retry-go and sethvargo/go-retry have no budget. The budget is retry's counterpart of the breaker's state machine: one per process and dependency, built at startup and shared by every goroutine. It's what justifies the package. 4. How the two-step API carries overWhat carries over: being inline. It matters even more for retry than for breaking, because whether a retry is safe depends on the request as well as the error. The method may not be idempotent, the request may carry an idempotency key, the body may not be replayable ( What doesn't carry over directly:
5. Why bedrock rather than humus
6. Draw the scope boundary explicitly
7. Design questions to settle before stories
|
Proposed API: the iterator, a user-provided classifier and a retry budgetThis proposes an API for retry. It builds on the two decisions settled after the review:
It answers the rest of the open questions. The four that needed the maintainer are settled too, under "Decided". How it was produced:
How it was checked:
Both are prototypes. Nothing is in the repository yet. The API at a glanceModule type Outcome int // Success, Retry, Stop; the zero value is invalid
type Decision int // Again, Succeeded, Stopped, Exhausted, Throttled, Expired; Done never returns zero
const TokenLimit = 1 << 16
type BudgetConfig struct { MaxTokens int; TokenRatio float64 } // both required
func NewBudget(cfg BudgetConfig) (*Budget, error)
func (b *Budget) Tokens() float64 // a pure read, for a gauge
func Exponential(base, maxDelay time.Duration) (func(attempt int) time.Duration, error)
func New(maxAttempts int,
schedule func(attempt int) time.Duration,
classify func(context.Context, error) (Outcome, time.Duration),
budget *Budget) (*Retrier, error) // budget may be nil
func (r *Retrier) Attempts(ctx context.Context) iter.Seq[*Attempt]
func (a *Attempt) Done(err error) Decision
func (a *Attempt) Number() int // from 1The one idiomvar err error
for a := range r.Attempts(ctx) {
err = call(ctx)
a.Done(err)
}
return err
Composing with circuitFrom the humus sketch. The breaker goes inside the loop, so each attempt is one call the breaker admits or rejects. The breaker's own idiom sits in a function per attempt, so its deferred func ClassifyHTTP(ctx context.Context, err error) (retry.Outcome, time.Duration) {
switch {
case err == nil:
return retry.Success, 0
case errors.Is(err, circuit.ErrRejected):
return retry.Stop, 0 // the breaker is telling the caller not to call
case errors.Is(err, ErrRewind):
return retry.Stop, 0
case ctx.Err() != nil:
return retry.Stop, 0
}
if se, ok := errors.AsType[*StatusError](err); ok {
switch se.Code {
case 429, 502, 503, 504:
return retry.Retry, se.RetryAfter
}
return retry.Stop, 0
}
return retry.Retry, 0 // a transport error
}
func (t Transport) RoundTrip(req *http.Request) (*http.Response, error) {
ctx := req.Context()
retryable := idempotent(req) && (req.Body == nil || req.Body == http.NoBody || req.GetBody != nil)
var resp *http.Response
var err error
for a := range t.Retrier.Attempts(ctx) {
attemptReq := req
if a.Number() > 1 {
// Only now is the last response replaced: if the context had
// ended during the wait, the loop would have ended with it.
discard(resp)
resp = nil
if attemptReq, err = rewind(req); err != nil {
a.Done(err) // an ErrRewind, which ClassifyHTTP stops on
break
}
}
resp, err = t.attempt(attemptReq)
if a.Done(result(resp, err)) != retry.Again || !retryable {
break
}
}
return resp, err
}
func (t Transport) attempt(req *http.Request) (*http.Response, error) {
ticket, err := t.Breaker.Allow(req.Context())
if err != nil {
return nil, err // a *circuit.RejectedError
}
defer ticket.Release()
resp, err := t.Next.RoundTrip(req)
ticket.Done(result(resp, err))
return resp, err
}A test pins the composition.
Answers1. What the classifier returns
2. How the caller learns why the loop stopped
3. Retry budget
4. Schedule
5. Validation, observer and clock
What the review changed
Concerns
DecidedThe maintainer settled these four, each as recommended:
Adoption costA fourth module:
Full API with doc comments (
|
Prototype source for the proposal aboveThe proposal above describes the prototype, and this is its library source, so that the stories can include it. The prototype lives in a session scratchpad, so this comment is its durable copy. The tests aren't included. They're 93 tests and examples at 100% statement coverage, and the stories will ask for that coverage again.
The @@ -22,6 +22,7 @@
- "!$test"
- "!${base-path}/fit/**"
- "!${base-path}/circuit/**"
+ - "!${base-path}/retry/**"
allow:
- $gostd
- github.com/z5labs/bedrock
@@ -30,6 +31,8 @@
desc: the bedrock module does not depend on the fit module
- pkg: github.com/z5labs/bedrock/circuit
desc: the bedrock module does not depend on the circuit module
+ - pkg: github.com/z5labs/bedrock/retry
+ desc: the bedrock module does not depend on the retry module
# Library code of the fit module imports only the standard library
# and OpenTelemetry's baggage. It does not import the bedrock module.
# Tests may use any module.
@@ -51,6 +54,16 @@
- "!$test"
allow:
- $gostd
+ # Library code of the retry module imports only the standard
+ # library. It does not import the bedrock module, the fit module or
+ # the circuit module. Tests may use any module.
+ retry:
+ list-mode: strict
+ files:
+ - "${base-path}/retry/**"
+ - "!$test"
+ allow:
+ - $gostd
forbidigo:
# Examples are documentation that users copy, so they are held to the
# same rule as everything else.
|
|
Closing as resolved. The design is settled, and it's filed as three stories. Each is blocked by the one before it:
Settled decisions:
The answers to the remaining design questions are in the API proposal. The stories embed the source they need. |
Uh oh!
There was an error while loading. Please reload this page.
For many of the same reasons as the circuit breaker request, I would like bedrock to provide a general retry package. I really like the 2-step api of the circuit breaker package and wonder if a similar api could be used for a retry package too.
All reactions