Add entity id to sharding props (#24053) #24058

talpr · 2017-11-24T11:14:12Z

Add overloads for ClusterSharding.start that take function from entity id to props.

I modified the tests that were included in the docs to use the id from props as well, let me know if you want me to revert that part...

akka-ci · 2017-11-24T11:16:03Z

Can one of the repo owners verify this patch?

johanandren · 2017-11-30T09:18:30Z

OK TO TEST

johanandren

Nice! Added some comments.

johanandren · 2017-11-30T09:25:36Z

akka-cluster-sharding/src/main/scala/akka/cluster/sharding/ClusterSharding.scala

+   */
+  def start(
+    typeName:        String,
+    propsGen:        ShardRegion.PropsGenerator,


I don't like it that it is called Factory in Java and Generator in Scala, could we just skip the function type alias instead, and call the param propsFactory here as well?

Sorry, I saw the differences between the MessageExtractor and the Scala function types and didn't realize that was because there's 2 functions in that interface. I'll change it.

johanandren · 2017-11-30T09:27:05Z

akka-cluster-sharding/src/main/scala/akka/cluster/sharding/ShardRegion.scala

+   */
+  trait PropsFactory {
+    def props(entityId: String): Props
+  }


Add the @FunctionalInterface annotation perhaps?

akka-ci · 2017-11-30T09:46:04Z

Test FAILed.

johanandren · 2017-11-30T10:14:50Z

Fails binary compatibility check, all changes are in places that shouldn't hurt though and can be filtered, the filter statements are in the failed build log and should go in a akka-cluster-sharding/src/main/mima-filters/2.5.7.backwards.excludes file, look at the other exclude files for an example

johanandren · 2017-11-30T10:20:53Z

Fixes #24053

talpr · 2017-11-30T17:05:30Z

@johanandren
Maybe I'm looking in the wrong place, but I can't find the filter statements you mentioned. The only thing I see in the log is:

[error] (akka-cluster-sharding/*:mimaReportBinaryIssues) akka-cluster-sharding: Binary compatibility check failed!

Followed by a stack trace. Can you point me to the right place?

johanandren · 2017-11-30T17:08:58Z

https://jenkins.akka.io:8498/job/pr-validator-per-commit-jenkins/10840/consoleFull
Search for ProblemFilters.exclude

akka-ci · 2017-12-01T09:20:07Z

Test FAILed.

akka-ci · 2017-12-01T09:53:46Z

Test FAILed.

akka-ci · 2017-12-01T10:24:39Z

Test PASSed.

ktoso

I think it deserves a small section explaining how to get the entity Id, be it via the path name trick or via the new method, then it's good to go :-)

ktoso · 2017-12-05T05:27:57Z

akka-cluster-sharding/src/main/scala/akka/cluster/sharding/ClusterSharding.scala

@@ -33,7 +32,7 @@ import scala.util.control.NonFatal
 import akka.actor.Status
 import akka.cluster.ClusterSettings
 import akka.cluster.ClusterSettings.DataCenter
-import akka.stream.{ Inlet, Outlet }
+import akka.cluster.sharding.ShardRegion.PropsFactory


ktoso · 2017-12-05T05:28:01Z

akka-cluster-sharding/src/main/mima-filters/2.5.7.backwards.excludes

@@ -0,0 +1,2 @@
+# 24058 - Add ClusterSharding.start overloads
+ProblemFilters.exclude[IncompatibleMethTypeProblem]("akka.cluster.sharding.ShardRegion.props")


patriknw

looking good, but a few small things

patriknw · 2017-12-05T09:23:53Z

akka-cluster-sharding/src/main/scala/akka/cluster/sharding/ShardRegion.scala

+   * from entity id.
+   */
+  @FunctionalInterface
+  trait PropsFactory {


why do we need this instead of using java.util.function.Function?

@patriknw In the first commit (before @johanandren's comments) I had this interface for Java and a type alias for Scala, to keep in line with the other params (ExtractEntityId, MessageExtractor, etc.). I also think having a specific type for it makes it clearer what it's for. But it isn't necessary, I can change it if you'd like.

I don't think it makes it clearer for such a simple function, it's another level of indirection to look at to understand what the parameter is about

I agree, I just didn't think that second step away from the original code :)

patriknw · 2017-12-05T09:28:13Z

akka-cluster-sharding/src/main/scala/akka/cluster/sharding/ClusterSharding.scala

+   */
+  def start(
+    typeName:           String,
+    propsFactory:       String ⇒ Props,


The name propsFactory is a bit vague. Props of what? I think it should still be entityProps, or entityPropsFactory

patriknw · 2017-12-05T09:35:34Z

akka-cluster-sharding/src/multi-jvm/scala/akka/cluster/sharding/ClusterShardingSpec.scala

@@ -39,13 +39,14 @@ object ClusterShardingSpec {
  case object Stop
  final case class CounterChanged(delta: Int)

-  class Counter extends PersistentActor {
+  def counterPropsFactory: String ⇒ Props = id ⇒ Props(new Counter(id))


is this showing up in documentation? I think the idiom should be to define it in the companion object, as we recommend for ordinary Props also. And I don't think there is a need to define the function there.

object Counter { val ShardingTypeName = "Counter" def props(id: String): Props = Props(new Counter(id)) }

and when registering it:

ClusterSharding(system).start(Counter.ShardingTypeName, Counter.props, ...

or in docs perhaps better to spell it out:

ClusterSharding(system).start(Counter.ShardingTypeName, entityId => Counter.props(entityId), ...

patriknw · 2017-12-05T09:37:38Z

akka-cluster-sharding/src/multi-jvm/scala/akka/cluster/sharding/ClusterShardingSpec.scala


  //#supervisor
  class CounterSupervisor extends Actor {
-    val counter = context.actorOf(Props[Counter], "theCounter")
+    val counter = context.actorOf(Props(new Counter("theCounter")), "theCounter")


Props should be defined in companion object according to our recommendations, e.g. avoid mistakes of closing over non thread safe things such as sender()

patriknw · 2017-12-05T09:41:07Z

akka-docs/src/main/paradox/cluster-sharding.md

+
+* Using the `propsFactory` parameter to `ClusterSharding.start`. The entity ID will be passed to the factory as a
+parameter, which can then be used in the creation of the actor (like the above example).
+* Using the actor's name (`self.path.name`). The actor's name will be the utf-8 URL-enconded entity ID.


with this new api added I see no value in describing the alternative of using the self.path.name

akka-ci · 2017-12-05T09:50:49Z

Test PASSed.

akka-ci · 2017-12-05T11:35:59Z

Test PASSed.

talpr · 2017-12-05T11:37:37Z

@patriknw @ktoso I think that should address everything,let me know if I've missed something

patriknw

additional nitpicks about the doc examples

patriknw · 2017-12-05T13:41:51Z

akka-cluster-sharding/src/main/scala/akka/cluster/sharding/ClusterSharding.scala

@@ -6,6 +6,7 @@ package akka.cluster.sharding
 import java.net.URLEncoder
 import java.util.Optional
 import java.util.concurrent.ConcurrentHashMap
+import java.util.function.Function


This is a bit scary since we are used to that Function is the Scala function.
we often rename that:

import java.util.function.{ Function => JFunction}

patriknw · 2017-12-05T13:46:35Z

akka-docs/src/test/java/jdocs/sharding/ClusterShardingTest.java

-    ActorRef startedCounterRegion = ClusterSharding.get(system).start("Counter",
-      Props.create(Counter.class), settings, messageExtractor);
+    ActorRef startedCounterRegion = ClusterSharding.get(system).start(Counter.ShardingTypeName,
+      Counter.propsFactory, settings, messageExtractor);


I think it's easer to read (understand) the docs by putting the function inline here

entityId -> Counter.props(entityId)

Actually, what about Counter::props for this?
(Should've seen that from the start, guess it's been a while since I wrote actual code in Java...)

that should work, but for the same reason I argued about using explicit entityId parameter in the scala examples we should do that here also

Alright, in that case this build should cover it (once it's done building)

patriknw · 2017-12-05T13:49:13Z

akka-docs/src/test/java/jdocs/sharding/ClusterShardingTest.java


    private final ActorRef counter = getContext().actorOf(
-        Props.create(Counter.class), "theCounter");
+            Counter.propsFactory.apply("theCounter"), "theCounter");


this would be weird, entities would get the id "theCounter"

The CounterSupervisor should also get its id from the factory parameter and pass that to Counter.props(entityId) here.

and similar in the scala example

That seemed odd to me too, but that's the way it was originally (self.path.name was "theCounter") so I didn't want to break anything. I'll change it.

patriknw · 2017-12-05T13:50:43Z

akka-docs/src/main/paradox/cluster-sharding.md

@@ -58,6 +58,10 @@ Scala
 Java
 :  @@snip [ClusterShardingTest.java]($code$/java/jdocs/sharding/ClusterShardingTest.java) { #counter-start }

+In some cases, the actor may need to know the `entityId` associated with it. This can be achieved using the `propsFactory`


propsFactory was renamed

patriknw · 2017-12-05T13:55:44Z

akka-cluster-sharding/src/multi-jvm/scala/akka/cluster/sharding/ClusterShardingSpec.scala

    import ShardRegion.Passivate

    context.setReceiveTimeout(120.seconds)

-    // self.path.name is the entity identifier (utf-8 URL-encoded)
-    override def persistenceId: String = "Counter-" + self.path.name
+    override def persistenceId: String = "Counter-" + id


s"${Counter.ShardingTypeName}-$id"

patriknw · 2017-12-05T13:56:25Z

akka-docs/src/test/java/jdocs/sharding/ClusterShardingTest.java

    @Override
    public String persistenceId() {
-      return "Counter-" + getSelf().path().name();
+      return "Counter-" + entityId;


ShardingTypeName + "-" + entityId

akka-ci · 2017-12-05T15:45:09Z

Test PASSed.

patriknw

LGTM

patriknw · 2017-12-06T06:35:34Z

Refs #24053

patriknw · 2017-12-06T06:36:31Z

thanks @talpr , it will be included in upcoming Akka 2.5.8

…ty-id-to-sharding-props" This reverts commit 8417e70, reversing changes made to 22e85f8.

* Revert "fix entityPropsFactory id param, #21809" This reverts commit cd7eae2. * Revert "Merge pull request #24058 from talpr/talpr-24053-add-entity-id-to-sharding-props" This reverts commit 8417e70, reversing changes made to 22e85f8.

* Revert "fix entityPropsFactory id param, akka#21809" This reverts commit cd7eae2. * Revert "Merge pull request akka#24058 from talpr/talpr-24053-add-entity-id-to-sharding-props" This reverts commit 8417e70, reversing changes made to 22e85f8.

akka-ci added the validating PR is currently being validated by Jenkins label Nov 30, 2017

johanandren reviewed Nov 30, 2017

View reviewed changes

akka-ci added needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Nov 30, 2017

talpr force-pushed the talpr-24053-add-entity-id-to-sharding-props branch from 586ccf8 to 839f04a Compare December 1, 2017 09:12

talpr force-pushed the talpr-24053-add-entity-id-to-sharding-props branch from 839f04a to 2f0c703 Compare December 1, 2017 09:39

talpr force-pushed the talpr-24053-add-entity-id-to-sharding-props branch from 2f0c703 to 62b610d Compare December 1, 2017 09:58

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Dec 1, 2017

ktoso suggested changes Dec 5, 2017

View reviewed changes

akka-ci added validating PR is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Dec 5, 2017

patriknw requested changes Dec 5, 2017

View reviewed changes

akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Dec 5, 2017

talpr force-pushed the talpr-24053-add-entity-id-to-sharding-props branch from 20ba758 to ffa7cf7 Compare December 5, 2017 11:10

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Dec 5, 2017

patriknw reviewed Dec 5, 2017

View reviewed changes

add entity id to sharding props (akka#24053)

a8e5f48

talpr force-pushed the talpr-24053-add-entity-id-to-sharding-props branch from ffa7cf7 to a8e5f48 Compare December 5, 2017 14:51

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Dec 5, 2017

patriknw approved these changes Dec 6, 2017

View reviewed changes

patriknw merged commit 8417e70 into akka:master Dec 6, 2017

patriknw mentioned this pull request Dec 7, 2017

fix entityPropsFactory id param, #21809 #24124

Merged

johanandren added a commit to johanandren/akka that referenced this pull request Dec 7, 2017

Revert "Merge pull request akka#24058 from talpr/talpr-24053-add-enti…

841e0c8

…ty-id-to-sharding-props" This reverts commit 8417e70, reversing changes made to 22e85f8.

johanandren mentioned this pull request Dec 7, 2017

Revert source incompatible sharding changes #24126

Merged

		@@ -0,0 +1,2 @@
		# 24058 - Add ClusterSharding.start overloads
		ProblemFilters.exclude[IncompatibleMethTypeProblem]("akka.cluster.sharding.ShardRegion.props")

Add entity id to sharding props (#24053) #24058

Add entity id to sharding props (#24053) #24058

Uh oh!

Conversation

talpr commented Nov 24, 2017

Uh oh!

akka-ci commented Nov 24, 2017

Uh oh!

johanandren commented Nov 30, 2017

Uh oh!

johanandren left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akka-ci commented Nov 30, 2017

Uh oh!

johanandren commented Nov 30, 2017

Uh oh!

johanandren commented Nov 30, 2017

Uh oh!

talpr commented Nov 30, 2017

Uh oh!

johanandren commented Nov 30, 2017

Uh oh!

akka-ci commented Dec 1, 2017

Uh oh!

akka-ci commented Dec 1, 2017

Uh oh!

akka-ci commented Dec 1, 2017

Uh oh!

ktoso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patriknw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akka-ci commented Dec 5, 2017

Uh oh!

akka-ci commented Dec 5, 2017

Uh oh!

talpr commented Dec 5, 2017

Uh oh!

patriknw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!