Automatic passivation for typed sharding #25765

johanandren · 2018-10-09T13:15:38Z

Fixes #25512

akka-ci · 2018-10-09T13:26:43Z

Test FAILed.

akka-ci · 2018-10-09T14:36:00Z

Test FAILed.

patriknw

looks like the right approach

some nitpick comments that you didn't ask for at this point..

...ster-sharding-typed/src/main/scala/akka/cluster/sharding/typed/ClusterShardingSettings.scala

patriknw · 2018-10-10T08:26:37Z

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

+  import context.dispatcher
+  val passivateIdleTask = if (settings.passivateIdleEntityAfter.toNanos > 0) {
+    val idleInterval = 1.second // FIXME
+    log.info("Idle entities will be passivated after {}", PrettyDuration.format(settings.passivateIdleEntityAfter))


Also didn't think it quite through, now it's logged once for every started shard. Should be logged from the region rather.

patriknw · 2018-10-10T08:27:32Z

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

  var passivating = Set.empty[ActorRef]
  val messageBuffers = new MessageBufferMap[EntityId]

  var handOffStopper: Option[ActorRef] = None

+  import context.dispatcher
+  val passivateIdleTask = if (settings.passivateIdleEntityAfter.toNanos > 0) {
+    val idleInterval = 1.second // FIXME


probably enough with half (or some other faction) of the total duration

patriknw · 2018-10-10T08:29:05Z

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

@@ -240,6 +256,9 @@ private[akka] class Shard(
    val id = idByRef(ref)
    idByRef -= ref
    refById -= id
+    if (passivateIdleTask.isDefined && lastMessageTimestamp.contains(id)) {


contains not needed here

patriknw · 2018-10-10T08:30:52Z

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

+    lastMessageTimestamp.foreach {
+      case (entityId, lastMessageTimestamp) ⇒
+        if (lastMessageTimestamp < deadline && refById.contains(entityId)) // test failed but, how could it not be in refs though?
+          passivate(refById(entityId), handOffStopMessage) // FIXME double check that this is the right message


yes, in Typed we use that, and that should be ok for untyped also

patriknw · 2018-10-10T08:32:18Z

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

-      messageBuffers.contains(id) match {
-        case false ⇒ deliverTo(id, msg, payload, snd)
+      if (passivateIdleTask.isDefined) {
+        lastMessageTimestamp += (id -> System.nanoTime())


I think this is slightly more efficient: lastMessageTimestamp = lastMessageTimestamp.updated(id, System.nanoTime())

johanandren · 2018-10-10T11:29:46Z

Not sure if I actually need a multi-JVM test for the feature given that it doesn't do any specific multi-node interactions, wdyt @patriknw ?

akka-ci · 2018-10-10T11:51:01Z

Test FAILed.

akka-ci · 2018-10-10T12:01:09Z

Test PASSed.

akka-ci · 2018-10-10T12:41:03Z

Test PASSed.

patriknw · 2018-10-10T16:41:59Z

@johanandren no need for multi-jvm test of this feature.

johanandren · 2018-10-10T17:04:43Z

Then it's ready for final review!

patriknw

looking good, some final touch...

...ster-sharding-typed/src/main/scala/akka/cluster/sharding/typed/ClusterShardingSettings.scala

patriknw · 2018-10-11T12:02:02Z

akka-cluster-sharding/src/main/resources/reference.conf

@@ -23,6 +23,10 @@ akka.cluster.sharding {
  # due to rebalance or crash.
  remember-entities = off

+  # Set this to a time duration to have sharding passivate entities when they have not
+  # gotten any message in this long time.
+  passivate-idle-entity-after = 0


would be nice to support "off" value also

patriknw · 2018-10-11T12:03:06Z

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

  var passivating = Set.empty[ActorRef]
  val messageBuffers = new MessageBufferMap[EntityId]

  var handOffStopper: Option[ActorRef] = None

+  import context.dispatcher
+  val passivateIdleTask = if (settings.passivateIdleEntityAfter.toNanos > 0) {


settings.passivateIdleEntityAfter > Duration.Zero

patriknw · 2018-10-11T12:06:16Z

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

@@ -265,6 +282,17 @@ private[akka] class Shard(
    }
  }

+  def passivateIdleEntities(): Unit = {
+    log.debug("Passivating idle entities")


I don't think that log statement adds much value. More useful would be to collect the entityIds to be passivated and log something like

log.debug("Passivating [{}] idle entities", idleEntities.size)

I did it like this to not have to create an intermediate collection, and since each passivation also logs at debug, but maybe having to correlate this log entry with those is a bad idea. I'll change it.

logging each passivation might be too much, that could be a storm of 1000s of log messages

if someone need that they can add the logging in their actor

That logging was already in place prior to this PR, you mean I should remove that?

hm, ok let's leave one of them "Entity stopped after passivation", but remove "Passivating started on entity"

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

patriknw · 2018-10-11T12:12:59Z

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

@@ -418,6 +418,8 @@ private[akka] class ShardRegion(
  // subscribe to MemberEvent, re-subscribe when restart
  override def preStart(): Unit = {
    cluster.subscribe(self, classOf[MemberEvent])
+    if (settings.passivateIdleEntityAfter.toMillis > 0)


> Duration.Zero

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

akka-cluster-sharding/src/test/scala/akka/cluster/sharding/InactiveEntityPassivationSpec.scala

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

patriknw · 2018-10-11T12:21:14Z

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

+The entities can be configured to be automatically passivated if they haven't received
+a message for a while using the `akka.cluster.sharding.passivate-idle-entity-after` setting,
+or by explicitly setting `ClusterShardingSettings.passivateIdleEntityAfter` to a suitable
+time to keep the actor alive. By default automatic passivation is disabled. 


do we need to mention that messages that are not sent via sharding are not counted, e.g. scheduled messages to self?

Good point, I'll add that.

akka-ci · 2018-10-12T12:12:28Z

Test PASSed.

chbatey

LGTM, some optional nits

...ster-sharding-typed/src/main/scala/akka/cluster/sharding/typed/ClusterShardingSettings.scala

akka-cluster-sharding/src/main/resources/reference.conf

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

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

akka-cluster-sharding/src/test/scala/akka/cluster/sharding/InactiveEntityPassivationSpec.scala

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

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

akka-ci · 2018-11-06T14:46:55Z

Test FAILed.

patriknw · 2018-11-06T15:08:09Z

it was formatting issue in InactiveEntityPassivationSpec.scala

akka-ci · 2018-11-06T17:37:04Z

Test PASSed.

johanandren requested a review from patriknw October 9, 2018 13:15

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

patriknw reviewed Oct 10, 2018

View reviewed changes

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

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 Oct 10, 2018

akka-ci added tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) labels Oct 10, 2018

johanandren changed the title ~~WIP Automatic passivation for typed sharding~~ Automatic passivation for typed sharding Oct 10, 2018

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 Oct 10, 2018

patriknw mentioned this pull request Oct 11, 2018

Enable passivate-idle-entity-after by default #25782

Closed

patriknw reviewed Oct 11, 2018

View reviewed changes

akka-ci added 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 Oct 12, 2018

chbatey approved these changes Oct 24, 2018

View reviewed changes

patriknw added the 2 - pick next Used to mark issues which are next up in the queue to be worked on. The tag is non-binding label Nov 5, 2018

johanandren added 9 commits November 6, 2018 14:11

Something along these lines?

20de25f

A little bit of typed API and some Java signatures as well

0a26ad8

Review feedback addressed

6f2a50d

Simple 1-node test added

39d8a8e

Bincomp. constructor added

020fed3

And some docs

56023c4

More review feedback addressed

78cbd31

Allow OFF

a4af6b8

Review feedback applied

a418e43

johanandren force-pushed the wip-25512-auto-passivation-sharding-johanandren branch from 4548353 to a418e43 Compare November 6, 2018 14:22

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

Formatting

27296b4

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 Nov 6, 2018

patriknw merged commit 133c413 into akka:master Nov 6, 2018

ismaelhamed mentioned this pull request Feb 8, 2019

Automatic passivation for sharding akkadotnet/akka.net#3718

Merged

Automatic passivation for typed sharding #25765

Automatic passivation for typed sharding #25765

Uh oh!

Conversation

johanandren commented Oct 9, 2018

Uh oh!

akka-ci commented Oct 9, 2018

Uh oh!

akka-ci commented Oct 9, 2018

Uh oh!

patriknw left a comment

Choose a reason for hiding this comment

Uh oh!

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!

johanandren commented Oct 10, 2018

Uh oh!

akka-ci commented Oct 10, 2018

Uh oh!

akka-ci commented Oct 10, 2018

Uh oh!

akka-ci commented Oct 10, 2018

Uh oh!

patriknw commented Oct 10, 2018

Uh oh!

johanandren commented Oct 10, 2018

Uh oh!

patriknw left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akka-ci commented Oct 12, 2018

Uh oh!

chbatey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!