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

BackoffSupervisor can't be used with ClusterSharding passivation #19016

Closed
masterzen opened this Issue Nov 25, 2015 · 7 comments

Comments

Projects
None yet
6 participants
@masterzen
Copy link

masterzen commented Nov 25, 2015

Despite 3f36423 that implements forwarding back messages from the child to the parent of the BackoffSupervisor (which is useful for passivation), this can't work because:

  1. The actor (protected by a BackoffSupervisor) needs to stop
  2. it sends a Passivate message to its parent BackoffSupervisor
  3. the parent BackoffSupervisor forwards it to the Shard
  4. the Shard sees the sender() as the real actor and not the BackoffSupervisor and finally executes calls passivate
  5. which in turn ends in this code with entity as the original passivation starter actor (the child of the BackoffSupervisor):
  def passivate(entity: ActorRef, stopMessage: Any): Unit = {
    idByRef.get(entity) match {
      case Some(id) if !messageBuffers.contains(id) 
        log.debug("Passivating started on entity {}", id)

        passivating = passivating + entity
        messageBuffers = messageBuffers.updated(id, Vector.empty)
        entity ! stopMessage

      case _  //ignored
    }
  }

The forwarded sender() can't be in the idByRef Map, since it's not the direct child of the Shard but the child of the BackoffSupervisor, so passivation is not started.

A related issue, if this aforementioned one is solved is that the real actor receiving the passivation return message will have to stop itself, but also to stop its ancestor (the BackoffSupervisor) otherwise this one will restart it.

In a word, passivation needs to be built-in in the BackoffSupervisor (or in a Cluster Sharding specific supervisor system), for it to work correctly.

patriknw added a commit that referenced this issue Nov 27, 2015

@patriknw patriknw added this to the 2.4.x milestone Nov 27, 2015

patriknw added a commit that referenced this issue Nov 27, 2015

Merge pull request #19027 from akka/wip-19016-BackoffSupervisor-passi…
…vation-patriknw

=act #19016 use the BackoffSupervisor as sender for parent msg
@ktoso

This comment has been minimized.

Copy link
Member

ktoso commented Nov 27, 2015

(note to self - keeping open until test added)

@nvollmar

This comment has been minimized.

Copy link
Contributor

nvollmar commented Dec 17, 2018

strange to see this in the release notes when the pull request has been merged 3 years ago already?

@chbatey

This comment has been minimized.

Copy link
Member

chbatey commented Dec 17, 2018

The PR was #25933

@nvollmar

This comment has been minimized.

Copy link
Contributor

nvollmar commented Dec 17, 2018

Right, thanks for the hint.

I looked through the change and tried to understand the impact of it. It only affects the backoff type StopImpliesFailure but the API suggests it applies to RestartImpliesFailure as well.

To have a more self-documenting API I would have expected it to be on the StopImpliesFailure as a case class instead or at least the documentation should make this more obvious.

@patriknw

This comment has been minimized.

Copy link
Member

patriknw commented Dec 18, 2018

@nvollmar thanks for feedback. Could you open a PR with the improvements?

@nvollmar

This comment has been minimized.

Copy link
Contributor

nvollmar commented Dec 19, 2018

@patriknw I'll look into it

@nvollmar

This comment has been minimized.

Copy link
Contributor

nvollmar commented Dec 19, 2018

@patriknw I've created a proposal: PR #26135
(Also addressed the potential issue I mentioned in PR #25933)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment