Akka cluster checkin

[Aaronontheweb]

Synchronizing some changes on the Akka.Cluster branch - I've been keeping everything regularly in-sync with dev but I figure it's time to share some of the work that's been done.

Completed thus far

1. Import ClusterMessages.proto #303 and Import ClusterMessageSerializer #304 are finished and covered with unit tests.
2. The Cluster ActorSystem extension itself is complete.
3. All of the Gossip and MetricsGossip plumbing, message types, and built-in actors have been completed. Covered with tests.
4. Metrics collection has been implemented using performance counters; covered with tests.
5. All of the ClusterSettings have been implemented, covered with tests.

Working on

1. @smalldave ported a giant chunk of the ClusterDaemon over, but there's still a lot of work to do there.
2. None of the cluster FailureDetection and Heartbeat stuff have been implemented yet
3. None of the cluster-specific routers have been implemented yet, and let's be honest: that's the cool stuff.
4. AutoDown has not been implemented yet.
5. None of the Multi-JVM tests have been ported yet.
6. ClusterActorRefProvider requires some work, but that's dependent on ClusterDaemon.

Stuff worth reviewing now

1. Scala has a convenient Number base class for all numeric types. C# doesn't have anything as nice as that. As a result, I decided to standardize all of our metrics collection on double types rather than do all of the crazy numeric conversion stuff that canonical Akka does. Going to add some line comments where relevant.
2. Inside the Metrics collector, there's a lot of Mono support stuff I am not sure about. CC @mattnischan. We added a reference to Microsoft.VisualBasic in order to get access to the ComputerInfo.TotalMemory metric, which is really just window dressing around a low-level SystemInfoEx (I think) P/Invoke call. We also use a couple of performance counters to get system available memory and CPU utilization. I have no about how Mono-compatible any of this code is, and I'm open to changing it before it gets released. For the time being I just needed something to get me started so I can work on ClusterDaemon and routing. See https://github.com/Aaronontheweb/akka.net/blob/a92a49df2f2e5eff4e8af526a2502d9cc5849c50/src/core/Akka.Cluster/ClusterMetricsCollector.cs
3. Microsoft.Bcl.Immutable is more or less an intractable part of Akka.Cluster in this design. Akka.Cluster doesn't do any networking or anything else - it's 80% state management and 20% state propagation to peers, and the ImmutableHashSet + ImmutableSortedSet make it much easier to handle gossip, vector clock, and node ring updates. The collections in Akka.Cluster actually do need to be immutable in most cases, since everything in the gossip protocol depends heavily on versioning. If we drop the Microsoft.Bcl.Immutable dependency we'll have to implement something like it ourselves, so it's worth considering the tradeoffs now.

[Aaronontheweb]

Oh, also - updated all of the specs to use @HCanber 's new TestKit. Changed all of the unit tests to support XUnit while I was at it (since I think that's what we've standardized on internally.)

[rogeralsing]

@Aaronontheweb you have to set it in the user config or a fallback on the user config.

The standard settings is parsed early on, so those properties are set before you can call InectTopLevelFallback or alter the remote or cluster config.

So create a spec baseclass that adds a fallback to the user fallback imo.

[HCanber]

I'm not sure how to review this. With 242 commits and 4932 changed files it's hard to see what has actually change. Would you mind updating akka-cluster to the latest in a separate PR and merge it (don't need to review that), and then rebase this? That would remove all irrelevant commits, right?

[Aaronontheweb]

Email reply did not go through. I don't think I can do that @HCanber - most of these changes are the original dev changes, and I can't squash commits that are already in the master repository.

[mattnischan]

In places like src/core/Akka.Cluster/ClusterSettings.cs, why aren't we using something like:

public readonly string SomeConfig { get; set; }

or

public string SomeConfig { get; private set; }

Either of those would get rid of the scores of private fields.

[mattnischan]

I also really think we should discuss moving to a one class per file structure. For example, how would I know to look for UniqueAddress in Member.cs, other than that's the way it is in the .scala original?

[rogeralsing]

I agree with @mattnischan, one class per file would feel more natural.
We did start doing this in the Akka assembly, however that is far from complete.

[Aaronontheweb]

@rogeralsing @mattnischan I use "Go to Declaration" or "Find Usages" in Visual Studio for that stuff.

But, I think it's a "it depends" scenario. Having a larger number of smaller files may not necessarily improve the readability, particularly when you're working on something like the ClusterMetricsCollector that has a lot of small moving parts all contained in the same file. Separating that stuff out, particularly when it's still being ported from Scala, would make it harder for me to figure out what's going on in that area.

But something that's important like UniqueAddress probably should get its own file. I think I did this in a few places in Akka.Remote too, like moving the EndpointManager out of Endpoints.cs.

I'm open to refactoring this into separate files after the code is stable, but in the short run I think it's easier for me to keep it the way Akka does while we're still porting it.

[Aaronontheweb]

@HCanber going forward I'll squash the commits on Akka.Cluster - there were A LOT of changes in dev this time that I had to integrate into Dave's work on Akka.Cluster before I could start work on new code, namely the testkit changes and porting everything to XUnit.

I'll also do some "drive-by" pull requests to pull this repository's dev branch into akka-cluster more often.

[Aaronontheweb]

@mattnischan that's a good question about ClusterSettings - I don't know why Dave wrote it that way. It wouldn't hurt anything to change it to auto properties with private setters though, since all of the conditional logic is handled at the time of the set.

[Aaronontheweb]

Yeah, so this pull request is virtually impossible to read. I'll keep it cleaned up next time but as I mentioned earlier, there was a lot of stuff that had to be pulled in from dev in order to move forward on it.

You can see the code cleanly here in my branch: https://github.com/Aaronontheweb/akka.net/tree/akka-cluster/src/core/Akka.Cluster

[mattnischan]

@Aaronontheweb I understand the need for keeping small parts together, but I would still argue that keeping to the canonical C# structure is more beneficial. Looking over ClusterMetricsCollector, each one of those classes is plenty big to be in its own file. To me, it's much more readable to see 50-100 line files than one huge 851 line file, especially if I'm working in an environment other than VS (and without search), which I have been for tackling the Mono stuff. In fact, many times I've been mucking around the files in vim on the command line because the linux dev box is headless, and its an enormous pain to try and find things.

I thinks this also brings up my earlier question, which is: are we porting the code or the spec?

[Aaronontheweb]

@mattnischan right now, the code as close as I can manage. I understand this type of clustering well enough to use it but not well enough to go too far off script at the moment.

[mattnischan]

@Aaronontheweb That's fair. I expect that we'll probably start housecleaning when we get closer to 1.0 also.

My fear is that we'll do it the way Akka in Scala does it, because that's the way to get the first pass done, but then never go back and re-investigate as we get more experienced, because there could be large regressions. But these aren't questions that we need to answer just yet, I don't suppose.

[Aaronontheweb]

@mattnischan it's fair to ask - it's worth remembering that the Akka Spec is a moving target too though, so this is always going to be an issue one way or another.

Our ability to make the code base readable and understandable has been improving along with our knowledge of how original Akka works, so I think that this is an issue we'll collectively become better at managing over time. Ideally I'd like things to be as close to idiomatic C# and F# as possible.

[Aaronontheweb]

Any objections to me merging this? Wrapping up work on the cluster heartbeat and failure detection stuff tomorrow and would love to start sending in smaller PRs.

[HCanber]

@mattnischan & @Aaronontheweb regarding Private readonly fields and getters

Example 1
private readonly string _value;
public string Value { get { return _value;} }

vs.

Example 2
public string Value { get; private set }

In the Example 1 the value is read-only and it cannot be changed, not even internally in the class.
The second example says nothing about the value beeing read-only, it only says that the value cannot be changed from the outside. So if you intend the value to be read-only this is a BAD choice.
If the value is meant to be readonly you should always use Example 1. That will prevent someone, in the future, from changing a value when they shouldn't have, and ultimately saving us from bugs.

@Aaronontheweb I wasn't talking primarily talking about squashing commits. I meant rebasing, and in worse case cherry-picking into an up-to-date-branch so that all your commits are ahead of dev. After that you can squash your commits.

[mattnischan]

@HCanber I understand the difference. But if the class is a configuration class with no members, isn't it a bit easier/cleaner to use a private auto-property and just leave a note? At least until C#6 comes around, which purportedly will have readonly auto-properties.

[HCanber]

@mattnischan I guess it comes down to personal preference. However, if someone comes a long and adds functions to it, like have been done with https://github.com/akkadotnet/akka.net/blob/334bc6b6cfdfbdd608c926ccaa0607b7291bec1b/src/core/Akka.Remote/RemoteSettings.cs. Should that person refactor all auto properties? It's very easy to forget. I prefer relying on code, than relying on people following comments. And as you say, it's only a configuration class, so I think it's easy enough to read, even with all those line. I would however put every property getter on one line. Readonly auto-props would be nice though.

[Aaronontheweb]

We should probably mark the setters on RemoteSettings as private or use readonly backing fields now that you mention it.

[Aaronontheweb]

@HCanber aha! I see - I think I've got that figured out now. I'm going to close this PR and open a new one.

[Aaronontheweb]

Ugh, screw this - I just lost all of yesterday's work when I messed up on my akka-cluster rebase (my fault) and now the new branch can't merge with akka-cluster. Pulling this. I promise the next one will be clean.

[smalldave]

Know this is closed but was away so never got the chance to comment.

My intention with the private fields was as @HCanber described. I too would prefer to have this right now rather than wait for language features that make this a little tidier.

Regarding class per file I'm not sure I have a concrete opinion but I'm in a similar position to Aaron in that I want to keep the port as close to the JVM version initially.

@mattnischan if you are using vim on a headless box have you looked at omnisharp? Gives you a lot of IDE like tools. I've only used it for small projects but it worked well. Pairing it with tmux and something like continuoustests makes for quite a nice development environment.

I'm very interested in mono support so anything in clustering to make that harder is simply because I haven't got that far yet.

[Aaronontheweb]

@smalldave @HCanber 's explanation makes total sense to me and I think we should use your style inside RemoteSettings too.