-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
data: Schedule #733
data: Schedule #733
Conversation
* @return | ||
* A computation that produces the result of this computation with Async and Abort[Throwable] effects | ||
*/ | ||
def retry(backoff: Int => Duration, n: Int)(using Flat[A], Frame): A < (S & Async & Abort[Throwable]) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the "raw" method to reduce API surface
/** The default retry policy with no backoff and 3 attempts. */ | ||
val default = Policy(_ => Duration.Zero, 3) | ||
/** The default retry schedule. */ | ||
val defaultSchedule = Schedule.exponentialBackoff(initial = 100.millis, factor = 2, maxDelay = 5.seconds) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More sensible defaults
86d3be6
to
5a5777c
Compare
* @return | ||
* The earlier of the two Instants. | ||
*/ | ||
def min(other: Instant): Instant = if instant.isBefore(other) then instant else other |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
infix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added infix
to the correspondent methods in Schedule
as well
val next = Maybe((interval, this)) | ||
def show = s"Schedule.fixed(${interval.show})" | ||
|
||
final case class Exponential(initial: Duration, factor: Double) extends Schedule: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all function based classes, would it make sense to have a single backing class?
I like the simplicity, but I wonder if this will be an issue for future binary compatibility. Regardless, should be a good amount of time before we need to focus on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok in terms of binary compatibility since these are internal APIs. The goal of the different classes is for Schedule
to automatically reduce to one of these canonical representations that can be compared for equality given that they're case classes. These tests show the functionality: https://github.com/getkyo/kyo/pull/733/files#diff-66ce2c17e23c73ff33f5e658a7ba3ef1972eb58d007873d50d795dae21a8e52dR578. I'm planning to use this behavior to optimize the execution of a new Timer
implementation based on Schedule
.
* Schedule provides various combinators for creating complex scheduling policies. It can be used to define retry policies, periodic tasks, | ||
* or any other time-based scheduling logic. | ||
*/ | ||
sealed abstract class Schedule derives CanEqual: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like that this is immutable and very generic - not specific to Kyo.
* @return | ||
* a tuple containing the next delay duration and the updated schedule | ||
*/ | ||
def next: Maybe[(Duration, Schedule)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not in love with this signature. In general, I am not a fan of Tuples...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I had a withNext
instead in one of the impls I pushed because of that. I decided to go back to the tuple because this is an API I'd expect users to rarely use directly and the tuple simplifies the implementation.
* Schedule provides various combinators for creating complex scheduling policies. It can be used to define retry policies, periodic tasks, | ||
* or any other time-based scheduling logic. | ||
*/ | ||
sealed abstract class Schedule derives CanEqual: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, how would you feel about self =>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in this case it wouldn't matter? There aren't nested classes where this
would be ambiguous
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall. We can revisit the tuple result later if necessary.
Inspired by ZIO's
Schedule
but as pure data structure.