Scheduled and repeat API + retry refactoring #172

micossow · 2024-07-03T15:17:33Z

closes #57

Summary:

retry* API has become more generic and renamed to scheduled*
existing retry* API uses scheduled* internally and retry-specific DSL has remained unchanged
new repeat* API also uses scheduled*` internally with repeat-specific DSL
scheduled API, Schedule trait and its implementations should be decoupled from retry API, specifically it calculates "duration" instead of "delay", which can later be interpreted as a delay or interval.
scheduled implementation is covered by tests for retry and repeat
docs updated where applicable

…Delay

…cheduled

adamw · 2024-07-05T13:13:34Z

core/src/main/scala/ox/resilience/retry.scala

+      policy.onRetry,
+      shouldContinueOnError = policy.resultPolicy.isWorthRetrying,
+      shouldContinueOnResult = t => !policy.resultPolicy.isSuccess(t),
+      delayPolicy = DelayPolicy.SinceTheEndOfTheLastInvocation


shouldn't the delay policy be part of RetryPolicy, so that it's also user-configurable if needed?

My idea was to create a public API runScheduled* with the all configurable options and make retry* and repeat* as "thin" as possible - basically an opinionated variants of runScheduled*, specifically:

retry - that behaves similarly to resillience4j retries, where sleep time is counted since the end of previous operation, has an optional onRetry callback and initial delay (sleep before first operation) is not relevant

repeat - that behaves similarly to Akka's Source.tick, where sleep time is counted since the start of previous operation (interval), doesn't expose onRepeat callback (not relevant) and initial delay is allowed
WDYT?

Ok, sounds good. Let's just document these defaults quite clearly, plus all the flexibility that runScheduled (or maybe simply scheduled?) gives you

adamw · 2024-07-05T13:14:03Z

core/src/main/scala/ox/scheduling/RepeatConfig.scala

+import scala.concurrent.duration.FiniteDuration
+import scala.concurrent.duration.DurationInt
+
+case class RepeatConfig[E, T](


we have RepeatConfig, but RetryPolicy - why?

RetryPolicy has been refactored to RetryConfig

adamw · 2024-07-05T13:14:15Z

core/src/main/scala/ox/scheduling/Schedule.scala

+  private[scheduling] sealed trait Finite extends Schedule:
+    def maxRepeats: Int
+    def initialDelay: FiniteDuration = Duration.Zero
+    def fallbackTo(fallback: Finite): Finite = FallingBack(this, fallback)


maybe .andThen would be a better name?

adamw · 2024-07-05T13:14:31Z

core/src/main/scala/ox/scheduling/Schedule.scala

+// TODO: shouldn't this trait be unsealed so that users can create their own schedules?
+sealed trait Schedule:
+  // TODO: consider better name that `attempt`
+  def nextDelay(attempt: Int, lastDelay: Option[FiniteDuration]): FiniteDuration


attempt - that's the attempt number, yes?

For retries it is an attempt number, for repeats it's an repetition number. I'm still looking for some common name :)

adamw · 2024-07-05T13:14:48Z

core/src/main/scala/ox/scheduling/Schedule.scala

+import scala.util.Random
+
+// TODO: shouldn't this trait be unsealed so that users can create their own schedules?
+sealed trait Schedule:


let's keep it sealed for now, and wait for a use-case to unseal it ;)

todo removed

adamw · 2024-07-05T13:15:34Z

core/src/main/scala/ox/scheduling/runScheduled.scala

+enum DelayPolicy:
+  case SinceTheStartOfTheLastInvocation, SinceTheEndOfTheLastInvocation
+
+case class RunScheduledConfig[E, T](


these classes/methods can all be private[ox] I suppose? they're not meant to be used by the end-user?

it has been decided that the scheduled and ScheduledConfig will be public API

adamw · 2024-07-05T13:17:07Z

Looks good but it would be great to see some examples of the API in action (in docs, or stand-alone) to see how the API can be used

adamw · 2024-07-09T20:39:09Z

core/src/main/scala/ox/scheduling/Schedule.scala

+import scala.util.Random
+
+sealed trait Schedule:
+  // TODO: consider better name that `attempt`


Yes, I think it would be the best in that case

adamw · 2024-07-09T20:41:06Z

core/src/main/scala/ox/scheduling/scheduled.scala

+/** The mode that specifies how to interpret the duration provided by the schedule. */
+enum SleepMode:
+  /** Interval (since the start of the last operation), i.e. guarantees that the next operation will start no sooner than the specified
+    * duration after the previous operations has finished. If the previous operation takes longer than the interval, the next operation will


started, not finished?

adamw · 2024-07-09T20:41:56Z

doc/adr/0008-scheduled-repeat-retry.md

@@ -0,0 +1,18 @@
+# 8. Retries


Good idea to add an ADR :)

kciesielski · 2024-07-10T05:47:49Z

doc/repeat.md

+The `repeat` config requires a `Schedule`, which indicates how many times and with what interval should the `operation` 
+be repeated.
+
+In addition, it is possible to define strategies for handling the results and errors returned by the `operation`:


question: Shouldn't we have a stronger division between retry, which is related to "trying and possibly failing" and repeat, which has no notion of errors? I think it would be simpler from the user perspective, otherwise they may be confused when to use retry and when to use repeat + shouldContinueOnError.

Right, I don't think that shouldContinueOnError handler would be used often.

shouldContinueOnError removed and mentioned that repeat can be used with retry + example

doc/scheduled.md

adamw · 2024-07-10T07:43:55Z

core/src/main/scala/ox/resilience/retry.scala

 import scala.util.Try

-/** Retries an operation returning a direct result until it succeeds or the policy decides to stop.
+/** Retries an operation returning a direct result until it succeeds or the config decides to stop.


I think I'd also mention here that retry is a special-case of schedule, with a given set of defaults (and enumarte these defaults). Maybe this should be in the @see section?

adamw · 2024-07-10T07:46:18Z

core/src/main/scala/ox/scheduling/Schedule.scala

+import scala.util.Random
+
+sealed trait Schedule:
+  def nextDuration(iteration: Int, lastDuration: Option[FiniteDuration]): FiniteDuration


or ... we're using invocation in the docs quite often, so maybe that would be a good name as well?

adamw · 2024-07-10T08:53:46Z

core/src/main/scala/ox/scheduling/Schedule.scala

+  private[scheduling] object FiniteAndFiniteSchedules:
+    /** A schedule that repeats indefinitely, using [[first]] first [[first.maxRepeats]] number of times, and then always using [[second]].
+      */
+    def forever(first: Finite, second: Infinite): Infinite = FiniteAndInfiniteSchedules(first, second)


is this correct that "finite and finite" schedule contains a method which takes a Finite and Infinite schedule?

yes, it doesn't make sense after refactoring. Fixed!

adamw · 2024-07-10T08:56:27Z

doc/repeat.md

+- `RepeatConfig.fixedRate[E, T](maxInvocations: Int, interval: FiniteDuration, initialDelay: Option[FiniteDuration] = None)`
+- `RepeatConfig.fixedRateForever[E, T](interval: FiniteDuration, initialDelay: Option[FiniteDuration] = None)`
+- `RepeatConfig.backoff[E, T](maxInvocations: Int, firstInterval: FiniteDuration, maxInterval: FiniteDuration = 1.minute, jitter: Jitter = Jitter.None, initialDelay: Option[FiniteDuration] = None)`
+- `RepeatConfig.backoffForever[E, T](firstInterval: FiniteDuration, maxInterval: FiniteDuration = 1.minute, jitter: Jitter = Jitter.None, initialDelay: Option[FiniteDuration] = None)`


it would be good to give an example of combinining schedules using .andThen (here and in the code snippet)

added some examples to scheduled.md and See [scheduled](scheduled.md) for details on how to create custom schedules. to repeat.md and retries.md

adamw

Just some minor adjustments, but looks great otherwise!

micossow added 2 commits July 3, 2024 17:17

refactor retry* functions to scheduled*

f5a0581

refactor Schedule.Every -> FixedRate

1c14d0e

micossow mentioned this pull request Jul 3, 2024

Add a schedule-aware repeat #57

Closed

micossow added 7 commits July 4, 2024 12:17

refactor schedule -> runScheduled and add repeat*

c449e30

fix compilation error

f526f74

fix compilation error and warnings

c27ed5d

fix doc compile

3728c62

add Schedule.InitialDelay, refactor Exponential.initialDelay -> first…

d511f02

…Delay

Schedule doc cleanup and disambiguation

373ee38

add todo

c68a9d6

micossow requested review from adamw and kciesielski July 5, 2024 07:19

micossow added 4 commits July 5, 2024 10:50

remove Schedule.FixedRate and move the start-to-start concept to runS…

686e152

…cheduled

fix default delayPolicy for retry

bfa4da0

add tolerance to fixedRate tests

a3e71b3

reduce sleep time in fixedRate tests

cc73acc

adamw reviewed Jul 5, 2024

View reviewed changes

micossow added 7 commits July 8, 2024 12:22

refactor runScheduled -> scheduled

d0d94b0

refactor RetryPolicy -> RetryConfig

6406eff

refactor Schedule.fallbackTo -> andThen

013bbdf

close todo

e1a5805

fix compileDocumentation

3ba7005

restore private modifier to Exponential.delay

90d2164

[doc] replace policy with config where applicable

8c3128e

micossow added 2 commits July 9, 2024 14:10

fix accidentally modified generated doc

e9d7660

reformat adr

e716e77

micossow requested a review from adamw July 9, 2024 12:17

micossow changed the title [WIP] Refactor retry* functions to scheduled* Scheduled and repeat API + retry refactoring Jul 9, 2024

micossow marked this pull request as ready for review July 9, 2024 12:18

adamw reviewed Jul 9, 2024

View reviewed changes

doc/adr/0008-scheduled-repeat-retry.md

@@ -0,0 +1,18 @@

# 8. Retries

Copy link

Member

adamw Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to add an ADR :)

kciesielski reviewed Jul 10, 2024

View reviewed changes

micossow added 2 commits July 10, 2024 09:16

fix doc

809275d

refactor attempt -> iteration in Schedule

8f9cbc7

micossow requested review from adamw and kciesielski July 10, 2024 07:22

adamw reviewed Jul 10, 2024

View reviewed changes

remove shouldContinueOnError from repeat

c966a90

adamw reviewed Jul 10, 2024

View reviewed changes

micossow added 6 commits July 10, 2024 10:12

[doc] update repeat doc about combination with retry

a893508

refactor variable/parameter names in scheduled

657534a

document to prefer to use repeat, retry instead of scheduled

0483281

[doc] mention how Retry/RetryConfig are special case of ScheduledConfig

6bfb177

scheduled: rename iteration -> invocation

493888f

fix doc compile

a25f23e

micossow requested a review from adamw July 10, 2024 08:52

adamw reviewed Jul 10, 2024

View reviewed changes

adamw approved these changes Jul 10, 2024

View reviewed changes

combining Schedules refactoring and examples

5b3290c

kciesielski approved these changes Jul 10, 2024

View reviewed changes

adamw merged commit c61beae into master Jul 10, 2024
5 checks passed

adamw deleted the 57-repeats branch July 10, 2024 10:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scheduled and repeat API + retry refactoring #172

Scheduled and repeat API + retry refactoring #172

micossow commented Jul 3, 2024 •

edited

Loading

adamw Jul 5, 2024

micossow Jul 8, 2024

adamw Jul 8, 2024

adamw Jul 5, 2024

micossow Jul 9, 2024

adamw Jul 5, 2024

micossow Jul 9, 2024

adamw Jul 5, 2024

micossow Jul 9, 2024

adamw Jul 5, 2024

micossow Jul 9, 2024

adamw Jul 5, 2024

micossow Jul 9, 2024

adamw commented Jul 5, 2024

adamw Jul 9, 2024

micossow Jul 10, 2024

adamw Jul 9, 2024

micossow Jul 10, 2024

adamw Jul 9, 2024

kciesielski Jul 10, 2024

micossow Jul 10, 2024

micossow Jul 10, 2024

adamw Jul 10, 2024

micossow Jul 10, 2024

adamw Jul 10, 2024

micossow Jul 10, 2024

adamw Jul 10, 2024

micossow Jul 10, 2024

adamw Jul 10, 2024

micossow Jul 10, 2024

adamw left a comment

Scheduled and repeat API + retry refactoring #172

Scheduled and repeat API + retry refactoring #172

Conversation

micossow commented Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamw commented Jul 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamw left a comment

Choose a reason for hiding this comment

micossow commented Jul 3, 2024 •

edited

Loading