Retriable errors and backoffs

[jsha]

I'm working on a tool that uses fetcher.go, and also calls GetProofByHash. I'd like to configure it to do backoffs and retries. I see that fetcher.go uses a Backoff struct provided internally:

certificate-transparency-go/scanner/fetcher.go

Lines 273 to 292 in 6ecca40

	bo := &backoff.Backoff{
	Min: 1 * time.Second,
	Max: 30 * time.Second,
	Factor: 2,
	Jitter: true,
	}
	var resp *ct.GetEntriesResponse
	// TODO(pavelkalinnikov): Report errors in a LogClient decorator on failure.
	if err := bo.Retry(ctx, func() error {
	var err error
	resp, err = f.client.GetRawEntries(ctx, r.start, r.end)
	return err
	}); err != nil {
	glog.Errorf("%s: GetRawEntries() failed: %v", f.uri, err)
	// There is no error reporting yet for this worker, so just retry again.
	continue
	}
	fn(EntryBatch{Start: r.start, Entries: resp.Entries})
	r.start += int64(len(resp.Entries))

But I think that usage might be broken. Backoff will only retry on grpc error codes that are retriable, plus the RetriableError struct. The GetRawEntries call I linked to is implemented by jsonclient.GetAndParse, which returns RspErrors. Those RspErrors don't match any of the conditions for retries.

I'd like to propose this: In addition to / instead of backoff.RetriableError, the backoff package should check for some interface, e.g. Retryable() bool. Then jsonclient.RspError can implement Retryable() as: a RspError is retryable if its wrapped error is a net.Timeout error OR the HTTP status code is >500.

What do you think?

[pav-kv]

Good catch, I think you are right. The retry will still happen because of the continue in line 289, but there will be no backoff in this case.