More aligned with other Akka library settings: Skip the builder and instead have factory create/apply methods with overloads. If there is ambiguity, we can have factory methods with more clear names for those (like RetrySettings.withFixedDelay(times, delay) perhaps).

The concrete RetrySettings then has the with methods to allow changing details after the construction.

Also: no case classes here unless internal API/hidden from users, highly problematic for bin comp evolvability with the copy method and traits it automatically extends.

I'd still hope for this, or something like it, to be the only implementation and that we can have a single concrete RetrySettings class and avoid having an ADT.

The only value the ADT brings as far as I can see is making it possible to inspect (lambdas would hide the backoff) and possibly change for example just the minBackoff after the fact, but I think we can live without those aspects.

For debuggability/logging we can make the predefined delay functions (exponential backoff, fixed delay, not sure there is anything more) concrete classes with good toString and concrete fields for parameters etc, rather than inline lambdas, while still keeping it completely generic here.

Ok, fair point, not sure about the last part:
For debuggability/logging we can make the predefined delay functions (exponential backoff, fixed delay, not sure there is anything more) concrete classes with good toString and concrete fields for parameters etc, rather than inline lambdas, while still keeping it completely generic here.

Do you think about sth like:

def apply(
      attempts: Int,
      minBackoff: FiniteDuration,
      maxBackoff: FiniteDuration,
      randomFactor: Double): RetrySettings = {
    new RetrySettings(
      attempts,
      attempted => Some(calculateExponentialBackoffDelay(attempted, minBackoff, maxBackoff, randomFactor))) {
      override def toString: String = {
        s"RetrySettings(attempts=$attempts, minBackoff=$minBackoff, maxBackoff=$maxBackoff, randomFactor=$randomFactor)"
      }
    }
  }

Almost, not subclassing the settings but having a concrete function class (could ofc be inlined as well):

class ExponentialBackoffFunction[R](min, max, etc) extends Function0[Future[R]] {
  def apply(): Future[R] = ... actual backoff ...
  override def toString = "ExponentialBackoff(min, max, etc)"
}

And then a good toString on RetrySettings including the delayFunction

aaaa, ok, let me try this out

Maybe: Feels natural that it'd be a separate parameter list in Scala, for this kind of usage:

retry(settings) { () => do it }

I flipped the order, but it's not consistent with existing methods.

Yeah, that's a good argument against, in streams which are newer than the APIs here we have a few that are more like my suggestion here,

Look at that btw, another case of pre-existing restart settings :)

I think it's ok to flip the ordering for this specific case even if now aligned with the other methods.

It's a much better API for Scala (like in streams)

ok, done

not sure if this is a good idea for a default behavior

I think we should expose exponential backoff as a separate and more direct:

def withExponentialBackoff(minBackoff: FiniteDuration, maxBackoff: FiniteDuration, randomFactor: Double): RetrySettings

Then make ExponentialBackoffFunction internal.

added, but keep in mind that we have a sort of creation dualism here:

RetrySettings(attempts, minBackoff, maxBackoff, randomFactor)

or

RetrySettings(attempts). withExponentialBackoff(minBackoff, maxBackoff, randomFactor)

maybe that's perfectly fine, but just wanted to align with the existing Akka conventions.

I don't think this is needed. For this simple case the ordinary withDelay function is enough.

For the record it was probably added because I insisted on a single RetrySettings class over an ADT of different retry settings, and to regain some debuggability by having concrete function impls with toStrings that would help get something useful out when logging/printing the retry settings (compared to an opaque lambda).

anyhow, if we keep the functions they should be internal

it's private now

Must not be a case class, for bincomp evolvability, we usually do that with an internal/private handcoded copy, see here for example

We likely will want the constructor private as well, and have it created through factories, because if not we will have to add chains of constructors calling each other every time we add a field.

ahh I always forget about bincomp

I think these could be private inside the RestartSettings to be properly hidden from Java, if not they need /** INTERNAL API */ @InternalApi

I don't remember, do we have something clever that will get classic scheduler from a typed actor system? If not we'd want a method with implicit ClassicActorSystemProvider so it can be used for typed as well (can probably not be an overload, but just having the provider version seems more convenient anyway).

a method with the implicit ClassicActorSystemProvider was added to Patterns, here I don't see any existing method with that, do you think it's worth adding it?

If we don't it won't be easy to use it from Scala based Akka typed apps, so better have that than the existing one (note that there is one classic Scheduler and one typed).

keep in mind that we have a sort of creation dualism here

yeah, seems unnecessary to have both factory methods and builder methods. Then we are back to zillions of overloaded methods again. I'd remove the factory methods (apply/create) and only leave the one with single parameter maxRetries: Int

Then used as:

RetrySettings(5).with...

the one with config can also be kept

done

I think this must be java.lang.Boolean for Java because it's a generic type

ok

these should still have the full internal api markers, because it's public for java

added