-
Notifications
You must be signed in to change notification settings - Fork 1k
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
ShardedDaemonProcess
push mode
#7229
ShardedDaemonProcess
push mode
#7229
Conversation
Thanks for fixing my dumb mistake in the test suite @Arkatufus - much appreciated lol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Detailed my changes - none of these API changes are breaking from a binary or source-compatibility standpoint, since no one could use return values from a void
method before anyway.
it is recommended to limit the nodes the sharded daemon process will run on using a role. | ||
This cluster tool is intended for small numbers of consumers and will not scale well to a large set. In large clusters it is recommended to limit the nodes the sharded daemon process will run on using a role. | ||
|
||
## Push-Based Communication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documented the two new pieces of functionality introduced here, which I'll review further down in the code itself
@@ -126,7 +132,9 @@ public void ShardedDaemonProcess_must_not_run_if_the_role_does_not_match_node_ro | |||
|
|||
var probe = CreateTestProbe(); | |||
var settings = ShardedDaemonProcessSettings.Create(Sys).WithRole("workers"); | |||
ShardedDaemonProcess.Get(Sys).Init("roles", 3, id => MyActor.Props(id, probe.Ref), settings, PoisonPill.Instance); | |||
var actorRef = ShardedDaemonProcess.Get(Sys).Init("roles", 3, id => MyActor.Props(id, probe.Ref), settings, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assert that we get a null
IActorRef
back here.
@@ -5,34 +5,41 @@ | |||
// </copyright> | |||
//----------------------------------------------------------------------- | |||
|
|||
#nullable enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every time I make API updates to an existing area of Akka.NET I always try to enable nullable
so it'll be less jarring when we make it the default in the future.
} | ||
|
||
public string Name { get; } | ||
public string[] Identities { get; } | ||
public IActorRef ShardingRef { get; } | ||
public ShardedDaemonProcessSettings Settings { get; } | ||
|
||
public ITimerScheduler Timers { get; set; } | ||
public ITimerScheduler Timers { get; set; } = null!; // gets set by Akka.NET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made compiler warning go away.
/// <see cref="ShardRegion"/> mechanism in order to piggyback off of its reliability and message buffering | ||
/// features, which <see cref="Router"/> and <see cref="ActorSelection"/> do not support. | ||
/// </remarks> | ||
internal sealed class DaemonMessageRouter : UntypedActor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new class - DaemonMessageRouter
. Performs round-robin message routing (according to the sending node - we don't care what other nodes who might be sending messages are doing) to all of the individual worker actors.
|
||
// have to remember to always allow the sharding envelope to be forwarded | ||
_shardingRef.Forward(new ShardingEnvelope(nextId, message)); | ||
if (_index == int.MaxValue) _index = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap-around if we're about to integer overflow.
public void Init(string name, int numberOfInstances, Func<int, Props> propsFactory) => | ||
/// <returns>A reference to a router actor that will distribute all messages evenly across the workers | ||
/// using round-robin message routing. <c>null</c> if the ShardedDaemonProcess is misconfigured.</returns> | ||
public IActorRef? Init(string name, int numberOfInstances, Func<int, Props> propsFactory) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified all of these methods to return an IActorRef?
- will return null
if we're the wrong role type to be hosting a worker and we'll also now log a WARNING
accordingly if that happens.
{ | ||
var sharding = ClusterSharding.Get(_system); | ||
var shardingRef = sharding.Start( | ||
typeName: $"sharded-daemon-process-{name}", | ||
typeName: FormatWorkerProcessName(name), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a shared function to name ShardedDaemonProcess
es and proxies - uses the exact same lexical structure as before so it won't break any existing deployments.
/// <param name="role">The role where the worker actors are hosted.</param> | ||
/// <returns>A reference to a router actor that will distribute all messages evenly across the workers | ||
/// using round-robin message routing.</returns> | ||
public IActorRef InitProxy(string name, int numberOfInstances, string role) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is new - since we added push-based communication to interact with ShardedDaemonProcess
es I thought it also made sense to leverage ShardRegionProxy
to allow non-hosting nodes to communicate with the worker processes. The only issue here is that if the user gets the number of entities wrong, then our routing logic will be skewed or broken. Not too different than if the user gets the message extractor, role, or entity type wrong so it's not a big deal IMHO.
@@ -4,7 +4,7 @@ | |||
// Copyright (C) 2013-2023 .NET Foundation <https://github.com/akkadotnet/akka.net> | |||
// </copyright> | |||
//----------------------------------------------------------------------- | |||
|
|||
#nullable enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enable nullable
on the settings class too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Changes
close #7195 - adds "push" mode to communicating with sharded daemon processes.
Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):
ShardedDaemonProcess
s #7195