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
2.1: fix sometimes-off-by-one in PeekMailbox, see #2851 #975
The head ref may contain hidden characters: "wip-2.1-2851-PeekMailbox-\u2202\u03C0"
Conversation
Started jenkins job akka-pr-validator at https://jenkins.akka.io/job/akka-pr-validator/249/ |
jenkins job akka-pr-validator: Success - https://jenkins.akka.io/job/akka-pr-validator/249/ |
@@ -40,15 +40,15 @@ class PeekMailboxExtension(val system: ExtendedActorSystem) extends Extension { | |||
class PeekMailboxType(settings: ActorSystem.Settings, config: Config) extends MailboxType { | |||
override def create(owner: Option[ActorRef], system: Option[ActorSystem]) = (owner, system) match { | |||
case (Some(o), Some(s)) ⇒ | |||
val tries = config.getInt("max-tries") ensuring (_ >= 1, "max-tries must be at least 1") | |||
val mailbox = new PeekMailbox(o, s, tries) | |||
val retries = config.getInt("max-retries") ensuring (_ >= 1, "max-retries must be at least 1") |
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.
Don't we normally use ConfigExceptions for this?
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.
yes, or IllegalArgumentException (possibly via requires)
ensuring throws assertion error, which was the reason for https://www.assembla.com/spaces/akka/simple_planner#/ticket:2798
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.
ah, yes, of course, my bad; will fix
Aside from comment: LGTM! |
LGTM |
2.1: fix sometimes-off-by-one in PeekMailbox, see #2851
👍 After the fact |
No description provided.