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

Port scala akka PR #26816 to Akka.NET #4511

Merged

Conversation

Arkatufus
Copy link
Contributor

Related to #4419
Port of akka/akka#26816

@Arkatufus Arkatufus marked this pull request as draft July 10, 2020 23:29
@Arkatufus Arkatufus changed the title Port scala akka PR #26816 to Akka.NET [WIP] Port scala akka PR #26816 to Akka.NET Jul 10, 2020
@Arkatufus
Copy link
Contributor Author

I'm having problem with this port, the port passes a scala ExecutionContext into the ActorSystem factory, and this ExecutionContext trickles down into the MessageDispatcher to be used inside the thread pool.
Problem is, I can't really emulate this with C#.
Any help will be greatly appreciated.

@Arkatufus
Copy link
Contributor Author

ActorSystem.Create() --> ActorSystemImpl() --> DefaultDispatcherPrerequisites().
Prerequisites object is then injected into the thread executor factory to be used as a configurator for each thread instantiation.

@Arkatufus
Copy link
Contributor Author

Should `Akka.Cluster.Metric" and "Akka.Remote" use internal dispatcher too?

@Arkatufus Arkatufus marked this pull request as ready for review July 14, 2020 22:02
@Arkatufus Arkatufus changed the title [WIP] Port scala akka PR #26816 to Akka.NET Port scala akka PR #26816 to Akka.NET Jul 14, 2020
@Aaronontheweb Aaronontheweb self-requested a review July 17, 2020 14:12
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.

Overall looks good, but have some important changes that need to be made.


## Dispatcher aliases

When a dispatcher is looked up, and the given setting contains a string rather than a dispatcher config block,
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@@ -262,7 +262,7 @@ public ClusterSharding(ExtendedActorSystem system)
{
var guardianName = system.Settings.Config.GetString("akka.cluster.sharding.guardian-name");
var dispatcher = system.Settings.Config.GetString("akka.cluster.sharding.use-dispatcher");
if (string.IsNullOrEmpty(dispatcher)) dispatcher = Dispatchers.DefaultDispatcherId;
if (string.IsNullOrEmpty(dispatcher)) dispatcher = Dispatchers.InternalDispatcherId;
Copy link
Member

Choose a reason for hiding this comment

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

LGTM - looks like all of the sharding /system actors (the guardians and coordinators) are supposed to run on the internal dispatcher

@@ -133,7 +133,7 @@ private IActorRef CreateReceptionist()
{
var name = _config.GetString("name");
var dispatcher = _config.GetString("use-dispatcher", null);
if (string.IsNullOrEmpty(dispatcher)) dispatcher = Dispatchers.DefaultDispatcherId;
if (string.IsNullOrEmpty(dispatcher)) dispatcher = Dispatchers.InternalDispatcherId;
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@@ -604,7 +605,9 @@ public static Props Props(Props singletonProps, ClusterSingletonManagerSettings
/// <returns>TBD</returns>
public static Props Props(Props singletonProps, object terminationMessage, ClusterSingletonManagerSettings settings)
{
return Actor.Props.Create(() => new ClusterSingletonManager(singletonProps, terminationMessage, settings)).WithDeploy(Deploy.Local);
return Actor.Props.Create(() => new ClusterSingletonManager(singletonProps, terminationMessage, settings))
.WithDispatcher(Dispatchers.InternalDispatcherId)
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@@ -70,7 +71,9 @@ public static Config DefaultConfig()
/// <returns>TBD</returns>
public static Props Props(string singletonManagerPath, ClusterSingletonProxySettings settings)
{
return Actor.Props.Create(() => new ClusterSingletonProxy(singletonManagerPath, settings)).WithDeploy(Deploy.Local);
return Actor.Props.Create(() => new ClusterSingletonProxy(singletonManagerPath, settings))
.WithDispatcher(Dispatchers.InternalDispatcherId)
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@@ -293,7 +293,7 @@ public void A_UnfoldResourceAsyncSource_must_use_dedicated_blocking_io_dispatche
var actorRef = refs.First(@ref => @ref.Path.ToString().Contains("unfoldResourceSourceAsync"));
try
{
Utils.AssertDispatcher(actorRef, "akka.stream.default-blocking-io-dispatcher");
Utils.AssertDispatcher(actorRef, ActorAttributes.IODispatcher.Name);
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@@ -336,7 +336,7 @@ private static ActorMaterializerSettings Create(Config config)
streamRefSettings: StreamRefSettings.Create(config.GetConfig("stream-ref")));
}

private const int DefaultlMaxFixedbufferSize = 1000;
private const int DefaultMaxFixedBufferSize = 1000;
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch

[Fact]
public void The_ActorSystem_must_provide_a_good_error_on_a_dispatcher_alias_loop_in_config()
{
Sys.Dispatchers.Invoking(d => d.Lookup("dispatcher-loop-1"))
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

task-peeking-mode = "FIFO"


dedicated-thread-pool{ #settings for Helios.DedicatedThreadPool
thread-count = 3 #number of threads
Copy link
Member

Choose a reason for hiding this comment

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

We might need to adjust this too.... The dedicated threadpool sized might interfere with these settings on the ForkJoinExecutor

src/core/Akka/Helios.Concurrency.DedicatedThreadPool.cs Outdated Show resolved Hide resolved
@Aaronontheweb
Copy link
Member

LGTM

@Aaronontheweb
Copy link
Member

Putting this on hold until we've completed #4537

@Aaronontheweb
Copy link
Member

Should `Akka.Cluster.Metric" and "Akka.Remote" use internal dispatcher too?

I need to check if Akka.Remote should use the same internal dispatcher - that's critical enough where it might merit having its own threads still.

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.

2 participants