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.

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

Sharding update #3524

Merged
merged 8 commits into from
Jun 25, 2018
Merged

Sharding update #3524

merged 8 commits into from
Jun 25, 2018

Conversation

zbynek001
Copy link
Contributor

@zbynek001 zbynek001 commented Jun 24, 2018

Contains following changes:

akka/akka#23472

Fix lookup of coordinator for sharding proxies
akka/akka#23995
AFAICT there was nothing ensuring the order of messages when sent to the
shard and the region so first checkthat the passivation has happened
before sending another add in the test
akka/akka#24013
@Aaronontheweb
Copy link
Member

@zbynek001 sure you don't want that tshirt?

@Aaronontheweb Aaronontheweb self-requested a review June 24, 2018 20:05
@Aaronontheweb
Copy link
Member

Seriously though, I was about to go through Cluster.Sharding this weekend to look at some of the open issues we have on that front :p - guess I'll start by reviewing this monster 👍

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.

Two minor details - beside that it looks good to go (also I'll have hell of a time trying to rebase it on multi-dc branch).

{
}
}

internal class CounterSupervisor : ActorBase
{
public readonly IActorRef Counter;
public static string ShardingTypeName => "CounterSupervisor";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be nameof(CounterSupervisor).


public static Config GetConfig()
{
return ConfigurationFactory.ParseString("akka.actor.provider = \"Akka.Cluster.ClusterActorRefProvider, Akka.Cluster\"")
Copy link
Contributor

Choose a reason for hiding this comment

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

Use akka.actor.provider = cluster.

@zbynek001
Copy link
Contributor Author

also wanted to include:

@Aaronontheweb I thought you were only joking ;)

@Horusiath Sorry about that, hope there will be not so many conflicts

@Aaronontheweb
Copy link
Member

@zbynek001 I have a PR open for some of those:

#3263

Think if I get this one passing that will fix it:

#3497

Copy link
Member

@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.

Couple of minor changes

@@ -0,0 +1,69 @@
//-----------------------------------------------------------------------
// <copyright file="ClusterShardingConfigSpec.cs" company="Akka.NET Project">
Copy link
Member

Choose a reason for hiding this comment

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

Copyright headers have the wrong file names

/// <returns>The actor ref of the <see cref="Sharding.ShardRegion"/> that is to be responsible for the shard.</returns>
public IActorRef Start(
string typeName,
Func<string, Props> entityPropsFactory,
Copy link
Member

Choose a reason for hiding this comment

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

So I think this API change is fine, but we should have some documentation about it on the cluster-sharding Akka.NET documentation similar to what they did on the JVM in that PR: https://github.com/akka/akka/pull/24058/files#diff-d0e085b72595aa4dfa9e30261feecd93

@Aaronontheweb
Copy link
Member

@zbynek001 I'm in the process of ordering some more tshirts, might take a bit to ship them but it's in the works. Send me an email with your shipping info :p

@Horusiath
Copy link
Contributor

@zbynek001 ActorSystem contains logger instance as well.

@zbynek001
Copy link
Contributor Author

also included this one:
ClusterSharding: automatically choose start or startProxy by a node role
akka/akka#23934

And that will be all in this PR

@zbynek001 zbynek001 changed the title [WIP] Sharding update Sharding update Jun 25, 2018
@Aaronontheweb
Copy link
Member

Great work as usual @zbynek001

@Aaronontheweb Aaronontheweb merged commit 41c116d into akkadotnet:dev Jun 25, 2018
@Aaronontheweb Aaronontheweb added this to the 1.3.9 milestone Jun 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants