Wip upgrade to 2.6.x √ #293

viktorklang · 2020-05-02T17:36:02Z

Lots of things to discuss here—but all-in-all this branch passes the TCK using the native-image generated.

build.sbt

viktorklang · 2020-05-02T18:07:51Z

build.sbt

+    assemblyMergeStrategy in assembly := {
+      /*ADD CUSTOMIZATIONS HERE*/
+      case PathList("META-INF", "io.netty.versions.properties") => MergeStrategy.last
+      case PathList(ps @ _*) if ps.last endsWith ".proto" => MergeStrategy.last


When we create "fat jars" it doesn't make sense to add the protos, as we don't expect anything to load them from the jar.

viktorklang · 2020-05-02T18:09:06Z

build.sbt

@@ -376,32 +413,30 @@ lazy val `proxy-core` = (project in file("proxy/core"))
        "io.grpc" % "grpc-netty-shaded" % GrpcJavaVersion,
        // Since we exclude Aeron, we also exclude its transitive Agrona dependency, so we need to manually add it HERE
        "org.agrona" % "agrona" % "0.9.29",
-        "com.typesafe.akka" %% "akka-remote" % AkkaVersion excludeAll (excludeTheseDependencies: _*),
+        akkaDependency("akka-remote"),
+        "com.typesafe.akka" %% "akka-protobuf-v390" % (AkkaVersion + "-proto390") excludeAll (excludeTheseDependencies: _*), // FIXME since akka-protobuf-v3 uses a shaded protobuf-java 3.10.0 which is not supported in native-image yet…


@patriknw special-cut this for us, as Protobuf-Java 3.10.0 uses VarHandles which are not fully working with GraalVM native-image 20.0.0 (and 20.1.0-dev).

@pvlugter @jroper @patriknw We need to decide on how to proceed with this—should we downgrade akka-protobuf to 3.9.0 or should we maintain an akka-protobuf-v390 artifact, or should we consider having akka depending on (and not shading) protobuf-java 3?

It looks like the usage of method handles was removed again in protobuf-java 3.11. So we may be able to instead upgrade akka-protobuf-v3 to 3.11?

Also discussed in protocolbuffers/protobuf#6497

I can try updating akka-protobuf-v3 to 3.11

akka/akka#29014

@pvlugter This cannot be fixed until Akka 2.6.6

viktorklang · 2020-05-02T18:10:06Z

graal-tools/src/main/scala/io/cloudstate/graaltools/AkkaSerializerRegisterFeature.scala

@@ -1,66 +0,0 @@
-package io.cloudstate.graaltools


This feature didn't work as intended, as it could not see what it needed to see, so I removed it until we can find a suitable replacement.

The issue here is that not all subtypes are reachable. But for those that are, it's a convenience to register their constructor (which otherwise mightn't be reached by any bytecode). (Side note: makes me wonder if registering the classes makes any sense, if the type is already reachable).

And for all the subtypes that are also used but not reached, I would just hard-code them here and reuse the same logic, instead of hard-code them verbosely in the config file.

proxy/core/src/main/resources/cloudstate-common.conf

tck/src/it/resources/application.conf

tck/src/it/scala/io/cloudstate/tck/TCK.scala

He-Pin · 2020-05-02T18:20:16Z

build.sbt

 val AkkaHttpVersion = "10.1.11"
 val AkkaManagementVersion = "1.0.5"
 val AkkaPersistenceCassandraVersion = "0.102"
 val PrometheusClientVersion = "0.6.0"
 val ScalaTestVersion = "3.0.5"
-val ProtobufVersion = "3.9.0"
+val ProtobufVersion = "3.9.0" // We use this version because it is the latest which works with native-image 20.0.0
 val GraalVersion = "20.0.0"


@hepin1989 2.1.0 will be released in 2 weeks, so holding off making modifications to GraalVM until then.

viktorklang · 2020-05-03T17:20:51Z

proxy/core/src/graal/META-INF/native-image/com.typesafe/ssl-config/reflect-config.json.conf

@@ -0,0 +1,6 @@
+[
+  {
+    "name": "com.typesafe.sslconfig.ssl.NoopHostnameVerifier"


@jroper @pvlugter We need to decide on what to choose for our usage—I have no idea and the documentation on the matter was not very helpful either. Right now it loads the NoopHostnameVerifier.

Depends on what it's being used for.

@patriknw Do you have any suggestion what what hostname verifier we should be using?

This is sslconfig. That isn't used for Akka remoting. It was deprecated in Akka 2.6. I would be surprised if it's used by Akka gRPC. @raboof do you know? Or is it still used in Akka HTTP?

pvlugter

I don't know anything about the graal setup, but changes look reasonable to me.

build.sbt

pvlugter · 2020-05-04T00:37:14Z

proxy/core/src/main/resources/cloudstate-common.conf

    sharding {
      rebalance-interval = 5s
+      passivate-idle-entity-after = off // FIXME put in a good value here


And remove the built-in passivation using receive timeout. Looks like cloudstate.proxy.passivation-timeout is currently 30s. Could substitute that setting here.

@pvlugter I suspect that we need to remove the receive timeout thing at the same time to not have competing timers?

Yes, remove the receive timeout at the same time. Although, I can imagine that passivation timeouts is something that will want to be defined per entity type on the user side? Maybe this needs its own issue to be enhanced.

@pvlugter Makes sense to make the change separately from this PR. (Hence the FIXME)

Sure. Would be good to capture in an issue then, rather than just a FIXME.

jroper · 2020-05-04T03:45:34Z

...-tools/src/main/scala/io/cloudstate/graaltools/ProtobufGeneratedMessageRegisterFeature.scala

@@ -11,9 +11,9 @@ import java.util.concurrent.ConcurrentHashMap
 final class ProtobufGeneratedMessageRegisterFeature extends Feature {


This should be modified. With the old version of protobuf that Akka used, the equals, hashCode and toString implementations used reflection, which is why every method needed a reflection configuration. But the new protobuf version they're on, when configured to be optimized for speed (rather than bytecode size), generates equals, hashCode and toString methods that don't use reflection. Furthermore, all of the akka.protobufv3.internal.* subtypes, Akka never loads them or interacts with them using reflection, rather, Akka has hand coded serializers that convert the Akka types to these types statically, and uses short manifest names, rather than classnames, with static mappings of these names to the protobuf types, when deserializisng. So, we should be able to just remove akka.protobufv3.* from here, no reflection configuration should be needed for it.

We still do need some config for the Cloudstate messages that we expect to be serialized by the Akka protobuf serializer, but I think there might be some problems with the code below, the Akka protobuf serializer uses parseFrom and toByteArray reflectively, but I don't see those being registered. Also, not sure how much of an overhead this is going to be to have configuration for classes that don't need it, we have a lot of protobuf classes that don't get loaded reflectively, only those sent over Akka remoting as top level messages need reflection configuration, which is only a handful of messages.

@jroper I'll try removing protobufv3 and report back

@jroper You were right! Removing the protobufv3 worked. I'll see if I can remove the need for the methods again.

@jroper It would be interesting to simplyfy this AutomaticFeature—but the challenge is the ridiculously long turnaround time to try even the smallest changes. Feel free (encouraged!) to find optimizations here! 👍

build.sbt

graal-tools/src/main/java/io/cloudstate/graaltools/Substitutions.java

viktorklang · 2020-05-04T18:10:14Z

@pvlugter This is ready to merge on my end.

pvlugter · 2020-05-04T23:40:20Z

This is ready to merge on my end

My preference would be to keep this open until the WIP and FIXMEs are addressed, so they're not forgotten or become comfortable hanging out in the codebase.

Co-authored-by: Viktor Klang (√) <[email protected]>

viktorklang · 2020-05-05T11:35:02Z

build.sbt

@@ -16,6 +16,7 @@ inThisBuild(
    organizationHomepage := Some(url("https://lightbend.com")),
    startYear := Some(2019),
    licenses += ("Apache-2.0", new URL("https://www.apache.org/licenses/LICENSE-2.0.txt")),
+    resolvers += "Akka Snapshots" at "https://repo.akka.io/snapshots/", // FIXME for akka-protobuf-v390


@pvlugter This cannot be fixed until Akka 2.6.6

viktorklang · 2020-05-05T11:36:16Z

proxy/core/src/main/resources/cloudstate-common.conf

@@ -3,6 +3,9 @@
 akka {
  actor {
    provider = cluster
+    allow-java-serialization = off
+    warn-about-java-serializer-usage = off
+    internal-dispatcher = akka.actor.default-dispatcher // FIXME consider removing this line


@pvlugter @patriknw Probably best solved by a decision—given that we control all that runs in this, I thouht it important to keep number of threads down. If you both agree with this setting, then I can remove the comment.

I think it's a valid choice for Cloudstate

Thanks @patriknw!

viktorklang · 2020-05-05T11:36:37Z

proxy/core/src/main/resources/cloudstate-common.conf

@@ -39,6 +42,8 @@ akka {
    artery {
      enabled = on
      transport = tcp
+      // FIXME do we need to set canonical.hostname?
+      // FIXME do we need to set advanced.tcp.connection-timeout?


@pvlugter @patriknw Should we set these two new settings, and to what?

default connection-timeout is 5s, so unless you think that is wrong you should start with defaults

if you don't set hostname it will be InetAddress.getLocalHost.getHostAddress, maybe good to have that as an optional environment variable here?

viktorklang · 2020-05-05T11:37:15Z

proxy/core/src/main/resources/cloudstate-common.conf

@@ -47,16 +52,17 @@ akka {
  cluster {
    shutdown-after-unsuccessful-join-seed-nodes = 60s

+    // FIXME consider setting distributed-data.max-delta-elements = <N>
+    // FIXME consider setting distributed-data.delta-crdt.max-delta-size = <N>


@pvlugter @patriknw These are here since they are a part of the migration docs, and I didn't want them to be forgotten, but I also don't know what to set them to. Advise?

Remove, and use defaults until you see a problem.
Only mentioned in migration guide because the defaults were changed (to better defaults).

viktorklang · 2020-05-05T11:39:17Z

@pvlugter There are no wip/fixmes in here that can currently be addressed (barring decisions needed)

proxy/core/src/main/resources/cloudstate-common.conf

patriknw · 2020-05-05T12:31:39Z

proxy/core/src/main/resources/cloudstate-common.conf

@@ -39,6 +42,8 @@ akka {
    artery {
      enabled = on
      transport = tcp


both these are default in Akka 2.6, so not strictly needed

patriknw · 2020-05-05T12:31:52Z

proxy/core/src/main/resources/cloudstate-common.conf

@@ -3,6 +3,9 @@
 akka {
  actor {
    provider = cluster
+    allow-java-serialization = off


default in Akka 2.6, so not strictly needed

patriknw · 2020-05-05T12:33:11Z

proxy/core/src/main/resources/cloudstate-common.conf

@@ -39,6 +42,8 @@ akka {
    artery {
      enabled = on
      transport = tcp
+      // FIXME do we need to set canonical.hostname?
+      // FIXME do we need to set advanced.tcp.connection-timeout?


default connection-timeout is 5s, so unless you think that is wrong you should start with defaults

patriknw · 2020-05-05T12:35:41Z

proxy/core/src/main/resources/cloudstate-common.conf

@@ -39,6 +42,8 @@ akka {
    artery {
      enabled = on
      transport = tcp
+      // FIXME do we need to set canonical.hostname?
+      // FIXME do we need to set advanced.tcp.connection-timeout?


if you don't set hostname it will be InetAddress.getLocalHost.getHostAddress, maybe good to have that as an optional environment variable here?

patriknw · 2020-05-05T12:37:21Z

proxy/core/src/main/resources/cloudstate-common.conf

@@ -47,16 +52,17 @@ akka {
  cluster {
    shutdown-after-unsuccessful-join-seed-nodes = 60s

+    // FIXME consider setting distributed-data.max-delta-elements = <N>
+    // FIXME consider setting distributed-data.delta-crdt.max-delta-size = <N>


Remove, and use defaults until you see a problem.
Only mentioned in migration guide because the defaults were changed (to better defaults).

viktorklang · 2020-05-05T13:34:58Z

proxy/core/src/main/resources/cloudstate-common.conf

@@ -3,6 +3,9 @@
 akka {
  actor {
    provider = cluster
+    allow-java-serialization = off


Suggested change

allow-java-serialization = off

proxy/core/src/main/resources/cloudstate-common.conf

Addressing FIXMEs in upgrade to Akka 2.6.5

pvlugter · 2020-05-06T00:14:03Z

Okay, captured a couple more follow-up issues in #297 and #298.

Work-in-progress on updating to Akka 2.6

4fae978

viktorklang requested review from jroper and pvlugter May 2, 2020 17:36

viktorklang force-pushed the wip-upgrade-to-2.6.x-√ branch 3 times, most recently from dfa7b33 to 30c24e3 Compare May 2, 2020 18:04

viktorklang commented May 2, 2020

View reviewed changes

build.sbt Show resolved Hide resolved

viktorklang commented May 2, 2020

View reviewed changes

build.sbt Show resolved Hide resolved

viktorklang commented May 2, 2020

View reviewed changes

build.sbt Show resolved Hide resolved

viktorklang commented May 2, 2020

View reviewed changes

proxy/core/src/main/resources/cloudstate-common.conf Outdated Show resolved Hide resolved

viktorklang commented May 2, 2020

View reviewed changes

tck/src/it/resources/application.conf Show resolved Hide resolved

viktorklang commented May 2, 2020

View reviewed changes

tck/src/it/scala/io/cloudstate/tck/TCK.scala Outdated Show resolved Hide resolved

He-Pin reviewed May 2, 2020

View reviewed changes

viktorklang force-pushed the wip-upgrade-to-2.6.x-√ branch 3 times, most recently from 6bed467 to 05b310d Compare May 3, 2020 17:11

viktorklang commented May 3, 2020

View reviewed changes

viktorklang force-pushed the wip-upgrade-to-2.6.x-√ branch from 05b310d to ac21d63 Compare May 3, 2020 17:31

pvlugter reviewed May 4, 2020

View reviewed changes

jroper reviewed May 4, 2020

View reviewed changes

dwijnand reviewed May 4, 2020

View reviewed changes

build.sbt Outdated Show resolved Hide resolved

dwijnand reviewed May 4, 2020

View reviewed changes

build.sbt Outdated Show resolved Hide resolved

viktorklang force-pushed the wip-upgrade-to-2.6.x-√ branch from ac21d63 to afa32ef Compare May 4, 2020 14:24

Upgraded to Akka 2.6.5

0467e03

viktorklang force-pushed the wip-upgrade-to-2.6.x-√ branch from afa32ef to 0467e03 Compare May 4, 2020 14:32

dwijnand reviewed May 4, 2020

View reviewed changes

graal-tools/src/main/java/io/cloudstate/graaltools/Substitutions.java Show resolved Hide resolved

Adding MODULE$ to SystemMaterializer$ reflection configuration

4e98b33

Apply suggestions from code review

c5fa618

Co-authored-by: Viktor Klang (√) <[email protected]>

viktorklang commented May 5, 2020

View reviewed changes

proxy/core/src/main/resources/cloudstate-common.conf Outdated Show resolved Hide resolved

patriknw reviewed May 5, 2020

View reviewed changes

viktorklang commented May 5, 2020

View reviewed changes

proxy/core/src/main/resources/cloudstate-common.conf Outdated Show resolved Hide resolved

viktorklang commented May 5, 2020

View reviewed changes

proxy/core/src/main/resources/cloudstate-common.conf Outdated Show resolved Hide resolved

viktorklang added 2 commits May 5, 2020 13:37

Apply suggestions from code review

2fddf75

Addressing FIXMEs in upgrade to Akka 2.6.5

Adding Akka+Node as ci test target

fb6e8f3

viktorklang force-pushed the wip-upgrade-to-2.6.x-√ branch from cde3d24 to fb6e8f3 Compare May 5, 2020 16:09

viktorklang merged commit 5565fa8 into master May 5, 2020

viktorklang deleted the wip-upgrade-to-2.6.x-√ branch May 5, 2020 17:56

		@@ -11,9 +11,9 @@ import java.util.concurrent.ConcurrentHashMap
		final class ProtobufGeneratedMessageRegisterFeature extends Feature {

Wip upgrade to 2.6.x √ #293

Wip upgrade to 2.6.x √ #293

Conversation

viktorklang commented May 2, 2020

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

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

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

pvlugter left a 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

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

viktorklang commented May 4, 2020

pvlugter commented May 4, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viktorklang commented May 5, 2020

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

pvlugter commented May 6, 2020