[WIP] [MNTK] Logging role names; less boilerplatish specs-coding process #2502

Rattenkrieg · 2017-02-05T01:13:51Z

Not ready for merge

Solution for #2469

PoC for discovery-time <node id - node role> mapping construction
Printing/reporting routines adjustment
Refactoring Akka.Remote.Tests.MultiNode
Refactoring Akka.Cluster.Tests.MultiNode
Refactoring Akka.Cluster.Tools.Tests.MultiNode
Solution to avoid depending on assemblies being tested in MultiNodeTestRunner
Unit-tests for new approach

Rattenkrieg · 2017-02-06T21:14:59Z

Got rid of remote.tests.multinode references.
More reflection was employed.
More command-line arguments were employed.
Ready for review. Tests on the way.

Aaronontheweb

First pass at a review. Mostly just have some questions thus far. Will look at the actual MNTR Program.cs in more detail next, but if you could answer my questions before then @Rattenkrieg that would be most helpful!

Aaronontheweb · 2017-02-07T16:48:19Z

build.fsx

@@ -319,6 +319,10 @@ Target "MultiNodeTests" <| fun _ ->
        let args = new StringBuilder()
                |> append assembly
                |> append "-Dmultinode.enable-filesink=on"
+                |> append "-Dmultinode.remote-testkit-dll=Akka.Remote.TestKit.dll"


So what's this for, telling the MNTR where to find the testkit assembly?

Will remove. For explanation check my comments below.

Aaronontheweb · 2017-02-07T16:49:16Z

src/core/Akka.MultiNodeTestRunner/Akka.MultiNodeTestRunner.csproj

@@ -73,14 +73,6 @@
      <Project>{964F0EC5-FBE6-47C5-8AE6-145114D5DB8C}</Project>
      <Name>Akka.MultiNodeTestRunner.Shared</Name>
    </ProjectReference>
-    <ProjectReference Include="..\Akka.NodeTestRunner\Akka.NodeTestRunner.csproj">


No need for the NodeRunner.exe any more?

I have deleted this by accident when was cleaning now unnecessary Akka.Remote.TestKit

Aaronontheweb · 2017-02-07T16:50:00Z

src/core/Akka.MultiNodeTestRunner.Shared/Sinks/MessageSink.cs

        protected static readonly Regex NodeFailureReasonRegex = new Regex(NodeFailureReasonRegexString);

        /*
         * Regular expressions - go big or go home. [Aaronontheweb]
         */
-        private const string NodeLogMessageRegexString = @"\[(\w){4}(?<node>[0-9]{1,2})\]\[(?<level>(\w)*)\]\[(?<time>\d{1,4}[- /.]\d{1,4}[- /.]\d{1,4}\s\d{1,2}:\d{1,2}:\d{1,2}(\s(AM|PM)){0,1})\](?<thread>\[(\w|\s)*\])\[(?<logsource>(\[|\w|:|/|\(|\)|\]|\.|-|\$|%|\+|#|\^|@)*)\]\s(?<message>(\w|\s|:|<|\.|\+|>|,|\[|/|-|]|%|\$|\+|\^|@|\(|\))*)";
+        private const string NodeLogMessageRegexString = @"\[(\w){4}(?<node>[0-9]{1,2})(?<role>:\w+)?\]\[(?<level>(\w)*)\]\[(?<time>\d{1,4}[- /.]\d{1,4}[- /.]\d{1,4}\s\d{1,2}:\d{1,2}:\d{1,2}(\s(AM|PM)){0,1})\](?<thread>\[(\w|\s)*\])\[(?<logsource>(\[|\w|:|/|\(|\)|\]|\.|-|\$|%|\+|#|\^|@)*)\]\s(?<message>(\w|\s|:|<|\.|\+|>|,|\[|/|-|]|%|\$|\+|\^|@|\(|\))*)";


We don't really use these Regex anymore, now that most node-specific logs are just written directly to their own file

But there is entire ParsingSpec in Akka.MultiNodeTestRunner.Shared.Tests using this regexp.

We can remove that spec.

Aaronontheweb · 2017-02-07T16:50:27Z

src/core/Akka.MultiNodeTestRunner.Shared/Sinks/ConsoleMessageSinkActor.cs

        {
            Console.ForegroundColor = ConsoleColor.Red;
-            Console.WriteLine("[NODE{0}][{1}]: SPEC FAILED: {2}", nodeIndex, DateTime.UtcNow.ToShortTimeString(), message);
+            Console.WriteLine("[NODE{0}:{1}][{2}]: SPEC FAILED: {3}", nodeIndex, nodeRole, DateTime.UtcNow.ToShortTimeString(), message);


Love this change. Having the rolename listed in the log will make things so much easier.

Rattenkrieg · 2017-02-07T20:53:08Z

Regarding dependency on Akka.Remote.TestKit:
I have implemented discovery routine using only names of types and loading Akka.Remote.TestKit assembly from file passed through commandline. Yesterday everything worked ok, but now when I'm running tests nothing being discovered, and in debug I'm getting weird exceptions. After investigation I have found that Akka.Remote.TestKit is sitting in my Akka.MultiNodeTestRunner/bin folder and interfering with Akka.Remote.TestKit which I'm looking somwhere in {AssemblyUnderTest}/bin. Guess where it came from?
This is the line from Akka.MultiNodeTestRunner.csproj:
<Compile Include="..\Akka.Remote.TestKit\CommandLine.cs">
So we are already depending on Akka.Remote.TestKit but in less obvious way.
I have decided to bring back reflection based on typeof(MultiNodeConfig) and sweeping flooded commandline args here
Code will be less fragile that way.

Rattenkrieg · 2017-02-08T18:46:10Z

Current fails looks transient. Need someone to confirm/re-run.
Everything planned is done, waiting for review.

Aaronontheweb · 2017-02-10T19:12:04Z

@Rattenkrieg have a merge conflict with the ClusterSingletonManagerLeaveSpec, which was updated in a recent PR.

Aaronontheweb · 2017-02-10T19:15:19Z

OMG, I love this

So much easier to figure out WTF is going on now that the role names are included in the per-node and in the aggregate logs.

Aaronontheweb · 2017-02-10T19:16:24Z

@Rattenkrieg so, binary compatibility question: this will require users to upgrade just the Akka.MultiNodeTestRunner package they're using, right? Or does the version of the Akka.Remote.TestKit they're using also have to be upgraded?

Rattenkrieg · 2017-02-10T19:26:53Z

@Aaronontheweb due to this change Akka.Remote.Testkit is also affected. But this parameter was added mostly for convinience. I can still manipulate with node index on that side though it makes me less confident about order being preserved.

Aaronontheweb · 2017-02-10T19:29:22Z

@Rattenkrieg eh, I don't think that change is the end of the world. Benefits outweigh the costs of upgrading IMHO.

As part of this PR, could you also update https://github.com/akkadotnet/getakka.net/blob/master/src/docs/testing/multinode-testkit.md ?

…ropagated to every output; remote tests assembly changed to respect current approach

Rattenkrieg · 2017-02-10T22:39:44Z

@Aaronontheweb rebased on head of dev, fixed conflicts. Now going to update the docs. As for regex foo you mentioned above is hard to track every caller of MessageSink but seems it all ends here and I can't see any Tell<string>(...) to that actor. According to that observations it's safe do delete this regex and everything down the road. But I think this is story for separate PR which I can address after finishing this one.

Horusiath · 2017-02-14T07:27:59Z

@Rattenkrieg My question regarding this PR: what scope of changes has affected visualizer? Will it show logs or barrier entries on the timeline? That would be super useful.

Rattenkrieg · 2017-02-14T12:28:33Z

@Horusiath That html file genereated is the product of "Visualizer"? If so then it was affected as everything where I had found usage of nodeid. Check the screenshot below:

Don't hesitate to ask for changes if I have missed some logs / or if you think that some logs could be done better.

Aaronontheweb reviewed Feb 7, 2017

View reviewed changes

Aaronontheweb added this to the 1.1.4 milestone Feb 10, 2017

Rattenkrieg force-pushed the mntk-role-names-2469 branch from d662e99 to 507dd90 Compare February 10, 2017 22:10

reflection-based approach to discovering node roles; node roles are p…

09db591

…ropagated to every output; remote tests assembly changed to respect current approach

Rattenkrieg force-pushed the mntk-role-names-2469 branch from 507dd90 to 09db591 Compare February 10, 2017 22:19

Rattenkrieg mentioned this pull request Feb 10, 2017

updated mntk docs akkadotnet/getakka.net#231

Merged

Merge branch 'dev' into mntk-role-names-2469

7eb17d7

Aaronontheweb merged commit fe2f30a into akkadotnet:dev Feb 14, 2017

Rattenkrieg mentioned this pull request Feb 18, 2017

Tech debt for 2502 - got rid of unused code #2522

Merged

Aaronontheweb mentioned this pull request Feb 23, 2017

[MNTK] Log node Role names instead of index #2469

Closed

[WIP] [MNTK] Logging role names; less boilerplatish specs-coding process #2502

[WIP] [MNTK] Logging role names; less boilerplatish specs-coding process #2502

Uh oh!

Conversation

Rattenkrieg commented Feb 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rattenkrieg commented Feb 6, 2017

Uh oh!

Aaronontheweb 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!

Rattenkrieg commented Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rattenkrieg commented Feb 8, 2017

Uh oh!

Aaronontheweb commented Feb 10, 2017

Uh oh!

Aaronontheweb commented Feb 10, 2017

Uh oh!

Aaronontheweb commented Feb 10, 2017

Uh oh!

Rattenkrieg commented Feb 10, 2017

Uh oh!

Aaronontheweb commented Feb 10, 2017

Uh oh!

Rattenkrieg commented Feb 10, 2017

Uh oh!

Horusiath commented Feb 14, 2017

Uh oh!

Rattenkrieg commented Feb 14, 2017

Uh oh!

Uh oh!

Rattenkrieg commented Feb 5, 2017 •

edited

Loading

Rattenkrieg commented Feb 7, 2017 •

edited

Loading