Pull request validation: SUCCESS 👍
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/652/

I might be missing something but why does the ClusterRouterGroup class need to make the decision? ClusterRouterSettingsBase already contains the useRole information so the Pool handling can generically just not create routees if that does not match.

By the time we come to the Pool spinning up actors it's too-late* as in "changes behaviour slightly". Leaving it completely up to the Pool is equal to my 2) up there. This introduces a small change in behaviour though:

Now:
scenario: start routees, max 1 on each node, allow local, use role "x", local node does not have role "x"
Router starts, RoutedActorCell executes:

      case pool: Pool ⇒
        if (pool.nrOfInstances > 0)    // the impl of this is not aware of cluster roles, returns maxNr!
          addRoutees(Vector.fill(pool.nrOfInstances)(pool.newRoutee(routeeProps, this)))

which starts nrOfInstaces routees on local even the local node does not match the useRole. This is the bug under discussion.

So the simple fix (2)) is to always return 0 in that nrOfInstaces method, of leaving starting routees 100% up to the Pool. RoutedActorCell will not start local routees "out of the blue". They will be started in reaction to MemberUp(theLocalNode). This code knows about the cluster and roles, the bug is solved. This introduces a delay, which was previously not there. Messages sent to the router, before it gets the MemberUp and starts the local routees will go to deadLetters, where as previously they would have landed in the "pre started local actors".

In summary and when thinking about it...
Maybe people should not expect the local routees to behave any differently than the remote routees.
This means instead of current state of this PR, we hardcode returning 0 in cluster pool router and the above scenario applies.

What I mean is that instead of returning 0 from nrOfInstances the calling code could just come to the same conclusion by inspecting the settings. And that code very likely does have an ActorSystem at its disposal.

But I’m fine with your conclusion as well; I don’t think any expectations are violated by not creating local routees when they are not called for ;-) And if it is easy to create the local routees eagerly nevertheless then we might do it just to be nice, but that is not required.

@rkuhn I think you are right that the calling code could figure it out, but the problem is that right now the calling code is from the general router code (not aware of such things). Not impossible to change though.

However, we should not rush this, we should think one more time about these kind of things related to the cluster aware routers (and I have swapped focus away this right now).

agreed

Totally agreed, don't want to rush this one, as there seems to be confusion around cluster-routers anyway (the #13802).

I'll try to dig up some time to hack on this more soon.

Pull request validation: SUCCESS 👍
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/744/

@ktoso I think that sounds good. Overall I think it would be good if the cluster aware pool could create the local routee early if allowLocal=true and the role is ok, because then you can start using the ActorRef immediately without loosing messages.

However, I think we should check the real cluster membership status and only do this if the node is Up. Currently that is not the case, it happily creates the routee assuming that the local node will become Up soon.
Note that you can grab this info with something like:

val selfAddress = Cluster(system).selfAddress
val isUp = Cluster(system).state.members.exists(m => m.address == selfAddress && m.status == MemberStatus.Up) 

@patriknw good hint about starting only when UP, incorporated this into the method ( ktoso@e870cbd#diff-5a96a7bdef4c972bff8f8f825b6640acR162 impl of it stays pretty much the same, if we go with 2nd proposal or 3rd proposal ).

I pushed another idea, which would boil down to having "SimplePool" and "ExtendedPool", with the idea that extended pool "can do whatever it needs", currently that's only during start() but it could be extended to do more... Now the question is, adding another interface here makes things a) (bad) inconsistent, we need to handle both Pool types everywhere b) (good) we could make the APIs a bit more specific...

So far I think the simplest would be the 2nd proposal, that is adding ActorSystem to nrOfInstances ( ktoso@9ea9f12 ), as 3rd seems to add too much stuff for sake of "maybe it will be used" hm.

Would you guys mind having a look again, so we can sattle on 2 or 3 and then I'll squash and do a migration guide?

I like the 2nd proposal.
Great that you explore different alternatives.

Pull request validation: FAILED 👎
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/748/

/note Didn't get enough time to revisit and clean it up properly today. Will re-visit it once back I guess.

Went with the above discussed 2nd approach (as in nrOfInstances(system)) and squashed everything.
Includes migration guide notice.

PLS BUILD

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/1078/

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/1079/

LGTM!!!!!!!

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/1081/

Fixed typo in migration guide and... ! :-)