Add side effect callbacks during retry #106
Conversation
There was a problem hiding this comment.
I don't think we need the beforeEachAttempt part, can't see any good use case for it. Users can simply enrich their retried effect with effectBefore >> myEffect and retry such composite effect. Adding this to the retry API could the flow a bit confusing. The cats-retry library offers a onError: (E, RetryDetails) => M[Unit] callback, and I think we should be fine with a similar thing. Wdyt @rucek @DybekK?
| afterEachAttemptInvoked shouldBe true | ||
| afterEachAttemptInvocationCount shouldBe 1 | ||
| returnedResult shouldBe Right(successfulResult) |
There was a problem hiding this comment.
Aren't those assertions redundant? If you verify that the callback was invoked once, I guess one of them would be sufficient. Same for the "before" callback.
There was a problem hiding this comment.
onRetryInvoked shouldBe truecan be removed, since checking the invocation count would be sufficient enough.returnedResult shouldBe Right(successfulResult)should not be removed. It checks whether the second parameter of theonRetryfunction has been given the correct result, in this case,Right(successfulResult). If this assertion fails, it indicates that theonRetryfunction wasn't given the correct result, and this oversight could potentially allow bugs to go unnoticed.
| @tailrec | ||
| def loop(attempt: Int, remainingAttempts: Option[Int], lastDelay: Option[FiniteDuration]): F[T] = | ||
| def sleepIfNeeded = | ||
| val delay = policy.schedule.nextDelay(attempt + 1, lastDelay).toMillis | ||
| if (delay > 0) Thread.sleep(delay) | ||
| delay | ||
|
|
||
| lifecycle.beforeEachAttempt(attempt + 1) |
There was a problem hiding this comment.
I'm wondering if we could just number the attempts starting from 1 instead of 0 - WDYT?
There was a problem hiding this comment.
I agree. In my opinion, it is more intuitive to read the code if we get rid of attempt + 1 in some places.
@kciesielski the difference would be that |
|
Yes, I'm aware of that difference, but I can't see the usefulness of this attempt number in such a case. I think we should choose a simpler API and sacrifice it. |
I completely removed |
|
Thanks! |
This PR addresses the proposed functionality in #56 and introduces enhancements to the retry mechanism by adding a
onRetryfunction callback to theRetryPolicyclass.onRetrycallback is executed after each retry attempt. It accepts two parameters: the attempt number and the result of the attempt. The result is represented as an Either type, where Left indicates an error and Right indicates a successful result. By default, this is an empty function.