Arvados

Probably we could reuse a preexisting library like https://pkg.go.dev/github.com/cloudflare/backoff

It's a pretty simple behavior to pull a whole module for (but arguably better than duplicating code). Yes, exponential backoff with jitter algorithm that is described there seems like what we want.

19972-go-client-retry @ 9baa7be4c57756cfef3a19139edbc54d398340e0 -- developer-run-tests: #3513

This uses the existing Timeout field as the maximum retry window. Arguably this is how the available time should be used -- if the application is willing to wait that long for a response, it probably doesn't care exactly where in the stack the waiting/retrying is happening, so existing code that uses Timeout probably wants this -- but OTOH it seems somewhat obfuscating. Is it important to let clients choose a shorter timeout for giving up on a single request but also auto-hangup-and-retry a request that takes longer?

• Update dispatcher (and other components as needed) to disable auto-retry

19972-go-client-retry @ 4aaff900e146c20c9e586223823e8661ce40d7f8 -- developer-run-tests: #3541

Tom Clegg wrote in #note-21:

19972-go-client-retry @ 4aaff900e146c20c9e586223823e8661ce40d7f8 -- developer-run-tests: #3541

This was a lot less invasive than I was worried it would be, and I really appreciate the design of the library you found. I'm sure it took time to make it this straightforward, thank you.

While I do like the library, it's under the MPL 2.0, which has some copyleft-ish requirements unlike the Apache License 2.0 we use for SDKs. I don't think there's a compatibility problem here, but it does put some additional requirements on us and other users of the SDK beyond the Apache License 2.0. As a matter of policy, are we okay with that?

In client.go, both shouldRetry and checkRetry seem to be unused. Am I missing something? Can we remove them?

Most of the "disable retry" lines look like:

        client.Timeout = 0 // disable auto-retry

… but both our coding guidelines and the Go style linked from there prefer separate comment sentences over inline comments. I think you wrote those guidelines, though, so I'm not gonna insist.

Not a review comment but a note for future readers: go-retryablehttpd uses exponential backoff by default, but you can configure the wait strategy using the Backoff field. If the outcome of #20107 is that we'd rather use jitter, that's implemented in the library too. We just switch the Backoff field to LinearJitterBackoff with MinWait=0.

Brett Smith wrote in #note-22:

the library you found.

I think Lucas found it... but yes, it did work out pretty well.

While I do like the library, it's under the MPL 2.0 [...] are we okay with that?

(From discussion) given that we already have other hashicorp MPL deps I think the answer is yes. (Plus, separately from this, we should continue to stay on top of licensing issues.)

In client.go, both shouldRetry and checkRetry seem to be unused. Am I missing something? Can we remove them?

Oops, yes. They were earlier versions of what's now in the retryablehttp library and in the inline func in Do. Removed.

prefer separate comment sentences over inline comments.

I personally think the inline form makes more sense for those mini comments, but indeed, the style guide currently says no, so no. Fixed.

19972-go-client-retry @ 47d1bc31bf9ee554665881c562ca2dea9acf9e22 -- developer-run-tests: #3548

Tom Clegg wrote in #note-23:

I personally think the inline form makes more sense for those mini comments

I agree, so I'm game to revise the coding guidelines if you are.

19972-go-client-retry @ 47d1bc31bf9ee554665881c562ca2dea9acf9e22 -- developer-run-tests: #3548

The changes look good to me, but that failed keep-balance test looks like it could actually be relevant? If it's just flaky, please add it to #20208, then go ahead and merge this. Thanks.

developer-run-tests-remainder: #3733