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
Typed: Un-nest supervision that overlaps #25269
Typed: Un-nest supervision that overlaps #25269
Conversation
).onFailure[RuntimeException](SupervisorStrategy.restartWithLimit(23, 10.seconds)) | ||
).onFailure[IllegalArgumentException](SupervisorStrategy.restart) | ||
).onFailure[RuntimeException](SupervisorStrategy.restart) | ||
}).onFailure[RuntimeException](SupervisorStrategy.stop) |
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.
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.
Pyramid of supervision doom!
} | ||
|
||
override def toString = s"restartWithLimit(${strategy.maxNrOfRetries}, ${strategy.withinTimeRange}" |
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.
missing closing )
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.
nice to strings in general though
Test PASSed. |
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
behavior match { | ||
case s: Supervisor[T, _] ⇒ | ||
val inner = loop(s.behavior) | ||
if (seenSupervised.contains(s.throwableClass)) |
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.
we could discard subclasses also but makes it a bit more complicated and the main purpose is to remove the accidental wrapping of same
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 thought about that, but wasn't sure it is worth because it isn't very realistic that you add more general and more general supervision over time.
inner | ||
else { | ||
seenSupervised += s.throwableClass | ||
if (inner.eq(s.behavior)) s |
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.
operator style of eq
} | ||
|
||
override def toString = s"restartWithBackoff(${strategy.minBackoff}, ${strategy.maxBackoff}, ${strategy.randomFactor})" |
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.
pretty duration?
Test PASSed. |
Test PASSed. |
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
Test PASSed. |
Fixes #25128
Also snuck in logging of the used strategy on failure in the supervision logs.