Retry functionality

[SystemFw]

Fixes #1459

I had originally started with a direct port of cats-retry, but since then I've changed the design significantly enough to warrant a proper review of the api before I port tests and docs.
Everything is in Retry.scala, here are some of the relevant decisions:

• The time scheduling part of the problem is mostly the same as cats-retry, except with fewer types.
• The retry api itself has changed significantly, cats-retry has several combinators, each of which takes several callbacks, whereas I've folded that into the Retry abstraction itself. I've found the deluge a similar combinators in cats-retry to be a bit hard to navigate, plus you can't have default implementations for the callbacks because they are of shape X => F[Y].
• I've removed the pretty printing of retries, we don't do that anywhere else in cats-effect.
• I've removed the ability to retry with a predicate on the result of the action, with the rationale that you can use failures and ensure instead.
• The errors are typed at Throwable (could be given the Gen* treatment though I'm not especially keen).

TODO:

• Syntax. Should be minimal given the reduction to a single retry combinator. I'm afraid usage with IO would require a syntax import since this lives in std.
• Pass at the scaladoc.
• Porting tests.

[armanbilge]

I'm afraid usage with IO would require a syntax import since this lives in std.

The core module depends on std, so syntax imports shouldn't be necessary, we can add methods directly to IO. Unless I misunderstood what you are getting at :)

[ybasket]

Maybe make the base (2) configurable? It could still be the default as many people will want to use it as their base.

[SystemFw]

I can probably add it as a param with a default, yeah

[armanbilge]

An overload is better for bincompat 🙏

[bplommer]

Or a builder? Overloads tend to lead to combinatorial explosion

[SystemFw]

By the way, I'm explicitly seeking feedback on the interaction between selectError and and/or with timed retry policies. I think in common usage it's fine, but I also think it's possible to build confusing behaviour in certain cases. However, the alternative is writing lots of overloads for retry combinators with callbacks, and forcing the user to bundle timing logic and error selection logic manually

[armanbilge]

I also think it's possible to build confusing behaviour in certain cases

would you mind showing an example of this?

[SystemFw]

would you mind showing an example of this?

For example capDelay used to be implemented as _.or(constantDelay(cap)). But if you use that with a policy that works on a specific error, e.g.

exponentialBackoff(1.ms)
  .selectError { 
       case NullPointerException => true
       case _ => false
    }.capDelay(500.ms) // with the old implementation of capDelay

the resulting policy will retry all errors, not just NullPointerException. Another example would be anding policies that select non-overlapping errors, like constantDelay.selectError(..ExA..).and(constantDelay.selectError(...ExB...), which means you never retry (though this one should be quite obvious with the rename from join to and).
All in all I think common usage is fine, since I'd imagine you first write the code that specifies the "scheduling" , and then selectError at the end, and also by following the simple rule of always having non-overlapping error selection for or, and overlapping error selection for and, but I think it's worth getting other people to think about it.

[bplommer]

Case classes tend to be fiddly from a bincompat perspective - could this be package-private and abstracted behind a sealed trait?

[djspiewak]

General thoughts…

I like where this is headed. It does feel slightly odd to me that we have all these combinators on Retry itself, but at the same time the only way to make a stateful decision chain is to do so using Retry.lift (which should be renamed but that's another topic). It feels like the set of combinators is incomplete somehow. Maybe the decision calculus is what's incomplete? I haven't given it too much thought.

[djspiewak]

I'm assuming OldRetry is only here for comparison purposes?

[djspiewak]

This feels like an odd use of the lift nomenclature. I probably would have gone with apply or something like that, though I'm not sure what to do about the non-functor variant.

[Daenyth]

Maybe def byStatus?

[djspiewak]

TIL people actually use exponential backoff without jitter. I had no idea that was even a useful thing to do…

[djspiewak]

Continuing on my thought from above… It feels like this should be the algorithm that we're encouraging people to reach for first, but the name definitely does not hint in that direction. I had to actually look up the AWS Engineering blog to figure out that this is actually the algorithm that I generally identify with "exponential backoff".

[Daenyth]

It feels a little odd to me that these are methods rather than some kind of Strategy that encapsulates the (previousBackoff: FiniteDuration) => (nextBackoff: F[FiniteDuration])

If they were structured in that way, we could provide easier guidance by having the strategies be available on object Strategy with def default being this recommended one

[djspiewak]

Why is this not a member of Retry?

[djspiewak]

Should we also add some convenience syntax for exponential backoff?

[kubukoz]

Should this use cats.effect.std.Random?