Add TransparentExponentialBackoffSupervisor #18776

henrymai · 2015-10-25T23:23:22Z

See this link for details:
#18766

akka-ci · 2015-10-25T23:26:03Z

Can one of the repo owners verify this patch?

patriknw · 2015-10-26T07:32:42Z

Can this be worked in to the BackoffSupervisor in a backwards compatible way? I don't think we should provide two classes for almost the same thing.

patriknw · 2015-10-26T07:32:58Z

Refs #18487

henrymai · 2015-10-26T08:00:50Z

I don't think there's a straight forward way to work my code into the BackoffSupervisor because it works in a fundamentally different way than BackoffSupervisor. Mine relies on the normal implicit supervision behavior where it uses a SupervisorStrategy.Decider to decide when to restart a child actor, whereas the existing BackoffSupervisor relies on the child being terminated in order to know to restart the child. This means that I actually support normal child termination without overloading the meaning as a signal to the supervisor to restart (see the Spec.scala for details).

There might be a way for me to do it the other way around, where I can provide a different props method that will pass a flag into the constructor, so that the actor can mimic old behavior as part of my supervisor. However, I don't see much value in doing so, because the old behavior is strictly less desirable than the new behavior. It would entail adding extra cruft into my implementation to support a behavior that is unwanted. Additionally the message interface that I expose is also different, in that I do not want to allow explicit retrieval of the child ActorRef.

I would prefer to just to keep these two separate and deprecate the existing BackoffSupervisor over time instead of attempting to merge the two together. It is cleaner this way and won't break any existing users of BackoffSupervisor.

rkuhn · 2015-10-26T17:51:42Z

akka-contrib/src/main/scala/akka/contrib/pattern/TransparentExponentialBackoffSupervisor.scala

+      maybeDirective.getOrElse(defaultDirective) match {
+        case Restart ⇒
+          self ! RestartChild
+          Stop


This Stop may overtake the RestartChild in the mailbox in the presence of other messages, leading to the termination of this supervisor actor.

Yea, I was afraid that might be the case.
But thankfully, I actually had an alternate implementation before that did a Resume here instead of a Stop (still sending a RestartChild message) and then in the RestartChild handler, it would terminate the child before the become(waitingToRestart(childRef, numRestarts)).
I'll switch it to this implementation later tonight, but I was wondering if you could elaborate a little more on why the Stop can overtake the RestartChild message in the presence of other messages (just for my own edification).

Actually, thinking about it, we might accidentally ensure that this cannot happen: Stop means a Terminate sysmsg to the child actor, which triggers a ChildTerminated sysmsg back to us, but processing that will just enqueue Terminated as a normal message at the back of the mailbox behind the RestartChild message—this is done so that any message from the child actor that was emitted before termination is also processed before the Terminated message and it saves the day here.

Still, it would be more obvious if you just switched the behavior to waitingToRestart right here and got rid of the RestartChild message altogether—initial creation can just happen in preStart.

but processing that will just enqueue Terminated as a normal message at the back of the mailbox behind the RestartChild message

Right, that's what I thought from reading the akka code.

Still, it would be more obvious if you just switched the behavior to waitingToRestart right here and got rid of the RestartChild message altogether—initial creation can just happen in preStart.

The problem with switching the behavior at that location is that I don't have access to numRestarts at that point (unless I make it a var). It is my preference to not use vars whenever I can avoid it.

This—in a nutshell—is the reason for making all lifecycle events (like child failure) normal messages in Akka Typed. I’d probably use a var here.

I just updated the patch to address this concern. Still managed to avoid using a var with the same lines of code :)

rkuhn · 2015-10-26T17:59:24Z

I agree with @henrymai, this implementation approach is superior to the existing one.

ktoso · 2015-10-26T18:56:22Z

akka-contrib/src/main/scala/akka/contrib/pattern/TransparentExponentialBackoffSupervisor.scala

+      val childRef = actorOf(props)
+      watch(childRef)
+      unstashAll()
+      become(watching(childRef, 0))


Nice, I like this impl.

patriknw · 2015-10-27T07:42:18Z

akka-actor/src/main/scala/akka/pattern/BackoffSupervisor.scala

@@ -61,6 +61,21 @@ object BackoffSupervisor {

  private case object StartChild extends DeadLetterSuppression
  private case class ResetRestartCount(current: Int) extends DeadLetterSuppression
+
+  def calculateDelay(


this should be private[akka] and marked as INTERNAL API

Addressed in the latest patch

patriknw · 2015-10-27T08:03:54Z

Can't we try harder to align the two backoff supervisors? I think it will be confusing for the users with two slightly different implementations. This implementation is not compatible with Akka Persistence (which must unconditionally stops the actor when there is a journal failure).

One idea is that Restart means ordinary restart and Stop means stop-backoff-start. Then we don't change semantics of restart and the needed semantics for Akka Persistence comes naturally.

henrymai · 2015-10-28T04:41:36Z

Hi Patrik,
Thanks for the review.

I think it will be confusing for the users with two slightly different implementations.

Can we solve that through documentation?

This implementation is not compatible with Akka Persistence (which must unconditionally stops the actor when there is a journal failure).

My implementation is targeting the case of standard actor and supervision semantics. Is there a reason why Akka Persistence went the non standard route of shutting down the child rather than throw a specific Exception/Throwable; allowing a supervising actor to decide to Stop for that specific exception?

One idea is that Restart means ordinary restart and Stop means stop-backoff-start. Then we don't change semantics of restart and the needed semantics for Akka Persistence comes naturally.

Unless I'm mistaken, persistFailure() will issue a context.stop(self), so it won't actually end up going through the SupervisorStrategy.decider. Meaning even if we decided to change the semantics of this patch to perform a "stop-backoff-start" for the Stop directive, we won't actually end up going through that path for Akka Persistence.

Is it not possible to just allow existing Akka Persistence users to continue using the BackoffSupervisor and explicitly document that the TransparentExponentialBackoffSupervisor is not suitable for usage with Akka Persistence?

rkuhn · 2015-10-28T09:08:20Z

As I said, @henrymai has a valid point here, the signaling mechanism is different so I don’t see how or why we should unify these. In particular I don’t like that self-stopping is part of the usage contract for BackoffSupervisor—that might work for specific cases (in particular “immortal” actors that come back after termination) but it is not the best general solution.

patriknw · 2015-10-28T11:47:49Z

@henrymai the reason persistent actors do context.stop(self) is that we can't enforce that users have installed a proper supervision strategy that performs the stopping (and stopping is unconditional as I mentioned earlier).

Ok, I'm in minority in thinking it will be confusing. Please work on the naming and documentation to make it clear then that there are two different utilities for this.

henrymai · 2015-11-03T17:58:15Z

@rkuhn @patriknw is there anything else that needs to be done for this to be merged?

rkuhn · 2015-11-07T16:55:56Z

Thanks for the contribution, @henrymai ! (and sorry for the long review delay)

Add TransparentExponentialBackoffSupervisor

rkuhn reviewed Oct 26, 2015
View reviewed changes

ktoso reviewed Oct 26, 2015
View reviewed changes

henrymai force-pushed the master branch 7 times, most recently from b006216 to 0759450 Compare October 27, 2015 05:34

patriknw reviewed Oct 27, 2015
View reviewed changes

henrymai force-pushed the master branch 2 times, most recently from 64b554b to 5b66b22 Compare November 1, 2015 20:25

Add TransparentExponentialBackoffSupervisor

a0e9b01

henrymai force-pushed the master branch from 5b66b22 to a0e9b01 Compare November 2, 2015 06:26

rkuhn added a commit that referenced this pull request Nov 7, 2015

Merge pull request #18776 from henrymai/master

61c257b

Add TransparentExponentialBackoffSupervisor

rkuhn merged commit 61c257b into akka:master Nov 7, 2015

henrymai mentioned this pull request Nov 7, 2015

Transparent exponential back off supervisor #18766

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TransparentExponentialBackoffSupervisor #18776

Add TransparentExponentialBackoffSupervisor #18776

henrymai commented Oct 25, 2015

akka-ci commented Oct 25, 2015

patriknw commented Oct 26, 2015

patriknw commented Oct 26, 2015

henrymai commented Oct 26, 2015

rkuhn Oct 26, 2015

henrymai Oct 26, 2015

rkuhn Oct 26, 2015

henrymai Oct 26, 2015

rkuhn Oct 26, 2015

henrymai Oct 27, 2015

rkuhn commented Oct 26, 2015

ktoso Oct 26, 2015

patriknw Oct 27, 2015

henrymai Nov 1, 2015

patriknw commented Oct 27, 2015

henrymai commented Oct 28, 2015

rkuhn commented Oct 28, 2015

patriknw commented Oct 28, 2015

henrymai commented Nov 3, 2015

rkuhn commented Nov 7, 2015

Add TransparentExponentialBackoffSupervisor #18776

Add TransparentExponentialBackoffSupervisor #18776

Conversation

henrymai commented Oct 25, 2015

akka-ci commented Oct 25, 2015

patriknw commented Oct 26, 2015

patriknw commented Oct 26, 2015

henrymai commented Oct 26, 2015

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

rkuhn commented Oct 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patriknw commented Oct 27, 2015

henrymai commented Oct 28, 2015

rkuhn commented Oct 28, 2015

patriknw commented Oct 28, 2015

henrymai commented Nov 3, 2015

rkuhn commented Nov 7, 2015