Initial replacement of Helios with DotNetty #2444

Horusiath · 2017-01-11T10:09:33Z

This PR contains changes necessary to replace existing Helios transport with DotNetty.

alexvaluyskiy · 2017-01-11T10:48:17Z

src/contrib/cluster/Akka.Cluster.Tools.Tests.MultiNode/ClusterClient/ClusterClientSpec.cs

@@ -611,7 +611,7 @@ public void ClusterClient_must_reestablish_connection_to_receptionist_after_serv
                    // start new system on same port
                    var sys2 = ActorSystem.Create(
                        Sys.Name,
-                        ConfigurationFactory.ParseString("akka.remote.helios.tcp.port=" + Cluster.Get(Sys).SelfAddress.Port).WithFallback(Sys.Settings.Config));
+                        ConfigurationFactory.ParseString("akka.remote.dot-netty.tcp.port=" + Cluster.Get(Sys).SelfAddress.Port).WithFallback(Sys.Settings.Config));


Maybe we should remove the dash and use dotnetty ?

Original name is DotNetty (which is PascalCase) therefore I've proposed dot-netty, as name convention for HOCON usually conforms to DashCase.

@Horusiath IMHO the - looks weird. I'd condense it down to just dotnetty too, although it doesn't really matter

alexvaluyskiy · 2017-01-11T10:51:03Z

Will you plan to support SSL and/or UDP in the initial version?

Horusiath · 2017-01-11T11:03:15Z

SSL will be supported. UDP probably won't - UDP is very useful when designing higher level protocol on top of it as it allows us to go faster than TCP (see QUIC or Aeron), but unless we build one by ourselves, UDP itself probably won't be useful enough for Akka.

Aaronontheweb · 2017-01-12T00:48:02Z

@Horusiath we should add UDP support anyway. Let the user make that decision. I get requests for it on occasion the time from IOT users.

Aaronontheweb · 2017-01-12T00:48:21Z

Although, doesn't need to be in THIS pull request. Can add it later.

Horusiath · 2017-01-18T14:59:36Z

I've added test certificate for SSL tests to .gitignore, however it seems that git ignores it anyway.

Horusiath · 2017-01-20T09:38:48Z

At this point there are still few remaining issues:

SSL support is working, but in order to be able to test it, I have to add a test certificate to certificate store. I'm adding/removing cert before/after the test itself. This allows us to correctly work with those tests in R# test runner. However this also requires to run VS with admin permissions. Same will apply to running build script on CI agent.
DNS resolution specs inherited from Helios are failing for unknown reason. They are FsCheck-based and can fail i.e. after 78 valid assertions. However when I'm trying to use the input data that caused an error and run this as a straightforward Xunit fact, everything works.
The last truly missing piece is compatibility with Helios. Right now when DotNetty transport detects akka.remote.helios.tcp, it can use to as primary source of configuration. This gives us backward compatibility for configuration. However actor systems using DotNetty and Helios at this moment still are unable to talk with each other - I'm guessing that the reason for that is the endianness of both (DotNetty → big endian, Helios → little endian), that needs to be adjusted. However the problem will be with further verification as none of the transport layers gives any useful info about exceptions and I haven't found a good hook point to insert breakpoint before that error happens.

Horusiath · 2017-01-23T10:58:45Z

At this point all tests are passing locally! Now we need to figure out how to enable secure socket layer tests working in CI agent in secure manner. I guess running pull request tests on agent with admin permissions is not the smartest move possible :P

alexvaluyskiy · 2017-01-23T12:30:50Z

Have you compared the performance between Helios and DotNetty?

alexvaluyskiy · 2017-01-23T12:34:28Z

DNS resolution specs inherited from Helios are failing for unknown reason.

There is a difference between Helios and DotNetty as I remember. DotNetty can't resolve 'localhost' @Aaronontheweb Am I Right?

alexvaluyskiy · 2017-01-23T12:35:58Z

The last truly missing piece is compatibility with Helios.

Do we need this compatibility? The best idea - to release a Helios transport as a separate package

Horusiath · 2017-01-23T12:45:52Z

@alexvaluyskiy I haven't made perf test yet. First I want CI build passing.

Resolving DNS is not a problem, as this is already implemented, in similar but not exactly the same way it works in Helios. And compatibility is desired, when people are upgrading their nuget packages, they expect to end up with working system. Helios will be released as a separate package (legacy transport layer), but I wanted to make DotNetty working as drop-in replacement.

I've managed to make DotNetty working with Helios already - major change was to switch default byte order in DotNetty to little endian (I've made it configurable).

Aaronontheweb · 2017-01-23T15:51:34Z

The backwards compatibility test still fails - is this more DNS stuff?

Horusiath · 2017-01-23T15:55:41Z

Nope, I've accidentally removed helios dependency from akka.remote.tests when fixing solution file.

Aaronontheweb

Have some questions and some requested changes

Aaronontheweb · 2017-01-23T15:53:25Z

src/contrib/transports/Akka.Remote.Transport.Helios/Akka.Remote.Transport.Helios.csproj

+    <Compile Include="HeliosTcpTransport.cs" />
+    <Compile Include="HeliosTransport.cs" />
+    <Compile Include="HeliosTransportSettings.cs" />
+    <Compile Include="Properties\AssemblyInfo.cs" />


Need to include SharedAssemblyInfo.cs, like the other projects, for versioning

Aaronontheweb · 2017-01-23T15:54:55Z

src/contrib/cluster/Akka.Cluster.Tools.Tests.MultiNode/ClusterClient/ClusterClientSpec.cs

@@ -611,7 +611,7 @@ public void ClusterClient_must_reestablish_connection_to_receptionist_after_serv
                    // start new system on same port
                    var sys2 = ActorSystem.Create(
                        Sys.Name,
-                        ConfigurationFactory.ParseString("akka.remote.helios.tcp.port=" + Cluster.Get(Sys).SelfAddress.Port).WithFallback(Sys.Settings.Config));
+                        ConfigurationFactory.ParseString("akka.remote.dot-netty.tcp.port=" + Cluster.Get(Sys).SelfAddress.Port).WithFallback(Sys.Settings.Config));


@Horusiath IMHO the - looks weird. I'd condense it down to just dotnetty too, although it doesn't really matter

Aaronontheweb · 2017-01-23T15:57:13Z

src/core/Akka.Remote.Tests/Transport/BackwardCompatibilitySpec.cs

+                    }
+                }");
+
+            using (var heliosSystem = ActorSystem.Create("helios-system", heliosConfig))


From the build server, fails on this line:

System.TypeLoadException : Cannot instantiate transport [Akka.Remote.Transport.Helios.HeliosTcpTransport, Akka.Remote.Transport.Helios]. Cannot find the type.

Aaronontheweb · 2017-01-23T15:58:31Z

src/core/Akka.Remote/Configuration/Remote.conf

@@ -310,14 +310,17 @@ akka {
      trttl = "Akka.Remote.Transport.ThrottlerProvider,Akka.Remote"
    }

-    ### Default configuration for the Helios based transport drivers
+	# necessary to keep backwards compatibility
+	helios.tcp.transport-class = "Akka.Remote.Transport.Helios.HeliosTcpTransport, Akka.Remote.Transport.Helios"


Based on the error above, this FQN probably isn't correct

Aaronontheweb · 2017-01-23T15:59:29Z

src/core/Akka.Remote/Transport/DotNetty/AkkaLoggingHandler.cs

+
+namespace Akka.Remote.Transport.DotNetty
+{
+    internal class AkkaLoggingHandler : ChannelHandlerAdapter


What's this class for? Debugging when using Akka.Remote?

Aaronontheweb · 2017-01-23T16:04:30Z

src/core/Akka.Remote/Transport/DotNetty/DotNettyTransport.cs

+            var resolved = await Dns.GetHostEntryAsync(address.Host);
+            //NOTE: for some reason while Helios takes first element from resolved address list
+            // on the DotNetty side we need to take the last one in order to be compatible
+            return new IPEndPoint(resolved.AddressList[resolved.AddressList.Length - 1], address.Port);


I don't understand this, but moreover we should probably take the first address that matches the preferred DNS family settings. It's probably a bug that we don't do this today for Helios in the first place.

I see that the overload below does this - does that get called by default?

It depends on enforce-ip-family setting. It IP address family enforcement is on, second option will be used. I also think, we should pick first correct address, this however was causing a failure between Helios and DotNetty backward compatibility spec - I don't know yet why.

Aaronontheweb · 2017-01-23T16:05:54Z

src/core/Akka.Remote/Transport/DotNetty/DotNettyTransport.cs

+        #endregion
+    }
+
+    internal class HeliosBackwardsCompatabilityLengthFramePrepender : LengthFieldPrepender


Is this ported over from the Helios 2.1 bits?

Yes. I've taken it from there + added ByteOrder.LittleEndian (because of DotNetty :) )

Aaronontheweb · 2017-01-23T16:07:01Z

src/core/Akka.Remote/Transport/DotNetty/DotNettyTransportSettings.cs

+                tcpKeepAlive: config.GetBoolean("tcp-keepalive", true),
+                tcpNoDelay: config.GetBoolean("tcp-nodelay", true),
+                backlog: config.GetInt("backlog", 4096),
+                enforceIpFamily: RuntimeDetector.IsMono || config.GetBoolean("enforce-ip-family", false),


I haven't checked yet, but do we know if the newer (4.6+) versions of Mono function correctly with IPV6 now?

Aaronontheweb · 2017-01-23T16:08:20Z

src/core/Akka.Remote/Transport/DotNetty/DotNettyTransportSettings.cs

+        public readonly int? WriteBufferHighWaterMark;
+        public readonly int? WriteBufferLowWaterMark;
+        public readonly bool BackwardsCompatibilityModeEnabled;
+        public readonly bool LogTransport;


This is for activating that Logging ChannelHandlerAdapter from earlier, right?

Aaronontheweb · 2017-01-23T16:09:40Z

src/core/Akka.Remote/Configuration/Remote.conf

-    helios.tcp {
+    ### Default configuration for the DotNetty based transport drivers
+
+    dot-netty.tcp {


Is there a new setting in here for turning on transport logging?

Yes. I've added log-transport value in order to add LoggingHandler to DotNetty pipeline. It's used to log user events on DotNetty side.

Aaronontheweb · 2017-02-03T19:01:12Z

Looking at a measured performance difference of about ~3k msg / s (edit: in favor of Helios) but I've seen that vary quite a bit on some of the benchmarks for this spec; been a difference sometimes as large as 6k.

The culprit to me looks like GEN 1 GC, which is roughly 10x is high as it is in the Helios transport. My guess is that @Horusiath is right as to the reason why: we're allocating a number of SwappedByteBuffer objects in the process of doing this.

Other possibility is that something like the log-transport option defaulting to true, although after looking through the code it shouldn't be the case.

maxcherednik · 2017-02-03T19:46:41Z

@Aaronontheweb @Horusiath guys, I haven't seen the exact changes here. Last time Aaron mentioned that dotnetty is nearly the same as Helios. I hope we keep the logging clean for the known case which we recently fixed on Helios.

…ibility spec for config

Horusiath added akka-remote api-change WIP labels Jan 11, 2017

Horusiath self-assigned this Jan 11, 2017

alexvaluyskiy reviewed Jan 11, 2017

View reviewed changes

Horusiath force-pushed the dotnetty branch 4 times, most recently from 64fca9b to f47ed27 Compare January 18, 2017 13:01

Horusiath force-pushed the dotnetty branch from f47ed27 to 651cdc2 Compare January 19, 2017 15:52

Horusiath force-pushed the dotnetty branch 3 times, most recently from 5febf78 to 8cd6fd4 Compare January 23, 2017 10:56

Horusiath force-pushed the dotnetty branch from 8cd6fd4 to 64775d5 Compare January 23, 2017 11:49

Horusiath force-pushed the dotnetty branch from 16d60c5 to 16bd29d Compare January 23, 2017 15:59

Aaronontheweb requested changes Jan 23, 2017

View reviewed changes

Horusiath force-pushed the dotnetty branch from 16bd29d to 7e2c75e Compare January 26, 2017 12:12

Horusiath force-pushed the dotnetty branch from 52c3766 to d62ba65 Compare February 4, 2017 09:22

Horusiath removed the WIP label Feb 4, 2017

Horusiath changed the title ~~[WIP] Initial replacement of Helios with DotNetty~~ Initial replacement of Helios with DotNetty Feb 4, 2017

Horusiath added 20 commits February 5, 2017 09:34

initial replacement of Helios with DotNetty

e2674a1

hack for client to not connet to IPAddress.Any

aff38fe

fixed public-hostname resolution

285d308

initialized ssl tests

cf9cc4a

integrated with DotNetty logging pipeline

8ff36e9

working ssl spec

7f91629

moved Helios transport to separate project + DotNetty backward compat…

3ad14f3

…ibility spec for config

working configuration compatibility between DotNetty & Helios

d9ec482

minor fixes

c3c4c94

fixed tests logging and ssl tests

dc450ec

setup byte-order for DotNetty transport

c0f9465

fixed compatibility spec

732f207

fixed sln file

8160b39

fixed MessageSerializerRemotingSpec

76735f0

fixed problems with F# projects

2d8bbfa

fixed DNS public hostname specs

74a960c

fixed F# files again

6a03b25

skipped ssl test for linux/mono

ccc1056

last polishing

5ac02e8

post-rebase fixes

6722539

Horusiath force-pushed the dotnetty branch from d62ba65 to 6722539 Compare February 5, 2017 09:01

Aaronontheweb merged commit c95b2d2 into akkadotnet:dev Feb 5, 2017

Aaronontheweb modified the milestone: 1.1.4 Feb 8, 2017

Aaronontheweb mentioned this pull request Feb 8, 2017

Implement DotNetty Transport #1356

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial replacement of Helios with DotNetty #2444

Initial replacement of Helios with DotNetty #2444

Horusiath commented Jan 11, 2017 •

edited

Loading

alexvaluyskiy Jan 11, 2017

Horusiath Jan 11, 2017

Aaronontheweb Jan 23, 2017

alexvaluyskiy commented Jan 11, 2017

Horusiath commented Jan 11, 2017

Aaronontheweb commented Jan 12, 2017

Aaronontheweb commented Jan 12, 2017

Horusiath commented Jan 18, 2017

Horusiath commented Jan 20, 2017 •

edited

Loading

Horusiath commented Jan 23, 2017

alexvaluyskiy commented Jan 23, 2017

alexvaluyskiy commented Jan 23, 2017 •

edited

Loading

alexvaluyskiy commented Jan 23, 2017

Horusiath commented Jan 23, 2017 •

edited

Loading

Aaronontheweb commented Jan 23, 2017

Horusiath commented Jan 23, 2017

Aaronontheweb left a comment

Aaronontheweb Jan 23, 2017

Aaronontheweb Jan 23, 2017

Aaronontheweb Jan 23, 2017

Aaronontheweb Jan 23, 2017

Aaronontheweb Jan 23, 2017

Aaronontheweb Jan 23, 2017

Aaronontheweb Jan 23, 2017

Horusiath Jan 23, 2017 •

edited

Loading

Aaronontheweb Jan 23, 2017

Horusiath Jan 23, 2017

Aaronontheweb Jan 23, 2017

Aaronontheweb Jan 23, 2017

Aaronontheweb Jan 23, 2017

Horusiath Jan 23, 2017

Aaronontheweb commented Feb 3, 2017 •

edited

Loading

maxcherednik commented Feb 3, 2017

Initial replacement of Helios with DotNetty #2444

Initial replacement of Helios with DotNetty #2444

Conversation

Horusiath commented Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexvaluyskiy commented Jan 11, 2017

Horusiath commented Jan 11, 2017

Aaronontheweb commented Jan 12, 2017

Aaronontheweb commented Jan 12, 2017

Horusiath commented Jan 18, 2017

Horusiath commented Jan 20, 2017 • edited Loading

Horusiath commented Jan 23, 2017

alexvaluyskiy commented Jan 23, 2017

alexvaluyskiy commented Jan 23, 2017 • edited Loading

alexvaluyskiy commented Jan 23, 2017

Horusiath commented Jan 23, 2017 • edited Loading

Aaronontheweb commented Jan 23, 2017

Horusiath commented Jan 23, 2017

Aaronontheweb 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

Horusiath Jan 23, 2017 • edited Loading

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

Aaronontheweb commented Feb 3, 2017 • edited Loading

maxcherednik commented Feb 3, 2017

Horusiath commented Jan 11, 2017 •

edited

Loading

Horusiath commented Jan 20, 2017 •

edited

Loading

alexvaluyskiy commented Jan 23, 2017 •

edited

Loading

Horusiath commented Jan 23, 2017 •

edited

Loading

Horusiath Jan 23, 2017 •

edited

Loading

Aaronontheweb commented Feb 3, 2017 •

edited

Loading