Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clustered routers now load correct default number of routees #2310

Merged
merged 1 commit into from
Sep 21, 2016
Merged

Clustered routers now load correct default number of routees #2310

merged 1 commit into from
Sep 21, 2016

Conversation

Aaronontheweb
Copy link
Member

closes #2266

@Aaronontheweb Aaronontheweb force-pushed the fix-2266-cluster-broadcast-router branch from d4c63ef to ffa0c8a Compare September 16, 2016 23:38
Aaronontheweb
Aaronontheweb commented Sep 16, 2016
View reviewed changes
Copy link
Member Author

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple changes that fix a big problem for all clustered group router users. They're fairly simple and I think I've explained them.

build.fsx
@@ -323,7 +323,7 @@ Target "MultiNodeTests" <| fun _ ->
|> append assembly
|> append "-Dmultinode.enable-filesink=on"
|> append (sprintf "-Dmultinode.output-directory=\"%s\"" testOutput)
|> appendIfNotNullOrEmpty spec "-Dmultinode.test-spec="
|> appendIfNotNullOrEmpty spec "-Dmultinode.spec="
Copy link
Member Author

@Aaronontheweb Aaronontheweb Sep 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allows the ability for the MNTR to correctly run a single spec from the FAKE commandline, i.e.

 ./build.cmd multinodetests spec=ClusterBroadcastGroup

src/core/Akka.Cluster.Tests.MultiNode/Routing/ClusterBroadcastRouter2266BugfixSpec.cs

namespace Akka.Cluster.Tests.MultiNode.Routing
{
public class ClusterBroadcastGroupSpecConfig : MultiNodeConfig
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multinode spec from @mrrd, based on the round-robin group clustered router MNTR spec.

Verifies that the router has the correct number of routees and that ALL of them receive each message.

src/core/Akka.Cluster.Tests/ClusterDeployerSpec.cs
@@ -85,6 +94,26 @@ public void RemoteDeployer_must_be_able_to_parse_akka_actor_deployment_with_spec
deployment.Dispatcher.ShouldBe("mydispatcher");
}

[Fact]
public void BugFix2266RemoteDeployer_must_be_able_to_parse_broadcast_group_cluster_router_with_default_nr_of_routees_routees()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduction spec which verifies that we weren't parsing the correct number of routees originally: https://github.com/akkadotnet/akka.net/blob/dev/src/core/Akka.Cluster/Configuration/Cluster.conf#L209

src/core/Akka.Cluster/ClusterActorRefProvider.cs
@@ -121,7 +121,7 @@ public override Deploy ParseConfig(string key, Config config)
&& !config.HasPath("nr-of-instances"))
{
var maxTotalNrOfInstances = config
.WithFallback(ClusterConfigFactory.Default())
.WithFallback(Default)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes the bug reported in #2266 - correctly pulls in the akka.actor.deplyoment.default section and has us search for the default cluster.max-total-nr-of-instances inside of there, which fixes the issue.

src/core/Akka/Actor/Deployer.cs
public class Deployer
{
private readonly Config _default;
protected readonly Config Default;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed to make the Default section visible to child classes.

@Aaronontheweb
Copy link
Member Author

Have a consistent cluster client spec failure... Not sure if it's just an issue with that spec or if I did something to affect it in this PR. Going to take a closer look now.

@Aaronontheweb
Copy link
Member Author

Looks like it's just an issue with the spec itself that's unrelated to these changes; was able to get it to pass locally and I reviewed the code as well and there's nothing in the ClusterClient that would be affected by these changes.

Horusiath
Horusiath reviewed Sep 19, 2016
View reviewed changes
Copy link
Contributor

@Horusiath Horusiath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 question - beside that it's good to go.

src/core/Akka.API.Tests/CoreAPISpec.ApproveCore.approved.txt
@@ -591,6 +591,7 @@ namespace Akka.Actor
}
public class Deployer
{
protected readonly Akka.Configuration.Config Default;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it need to be here as an instance field? It looks repetitive - maybe place it somewhere else or use as a static?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a private field before. Doesn't need to be static since there's only ever 1 instance per ActorSystem, but multiple actor systems could have different values for this field depending on which plugins they loaded (although it looks like only Akka core and Akka.Cluster really use it at the moment.)

@Aaronontheweb
Copy link
Member Author

@Danthar doh - looks like the new "Update Branch" button Github added on PRs creates a merge commit. I'll rebase that...

@Aaronontheweb Aaronontheweb force-pushed the fix-2266-cluster-broadcast-router branch from 600981c to 091ac66 Compare September 20, 2016 20:01
@Aaronontheweb
Copy link
Member Author

Rebased

@alexvaluyskiy
Copy link
Contributor

@Aaronontheweb rebase it again, please

@Aaronontheweb Aaronontheweb force-pushed the fix-2266-cluster-broadcast-router branch from 091ac66 to 984d527 Compare September 21, 2016 07:21
@Aaronontheweb
Copy link
Member Author

@alexvaluyskiy done

@Danthar
Copy link
Member

Danthar commented Sep 21, 2016

mono CI failing is expected?
And we have a failing unit test: RoutingSpec.Routers_in_general_must_evict_terminated_routees

@Aaronontheweb
Copy link
Member Author

Mono specs failed because of xunit/xunit#979 - might be another issue where the Mono runtime can randomly crap out inside XUnit, similar to this bug that I reported: https://bugzilla.xamarin.com/show_bug.cgi?id=44546

As for the routee evicted spec - looks like it just timed out. Can happen sometimes when it gets busy. None of the code here affects local routers since they were already using the configuration data that now also gets exposed to clustered routers.

@Danthar Danthar merged commit 458daa9 into akkadotnet:dev Sep 21, 2016
@Aaronontheweb Aaronontheweb deleted the fix-2266-cluster-broadcast-router branch September 21, 2016 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Horusiath Horusiath Horusiath left review comments

Assignees
No one assigned
Labels
rebase required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Clustered Broadcast Group Routers do not seem to work correctly
4 participants
@Aaronontheweb @alexvaluyskiy @Danthar @Horusiath