-
Notifications
You must be signed in to change notification settings - Fork 22
WithDistributedData()
will start DistributedDataProvider
extension automatically.
#597
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
WithDistributedData()
will start DistributedDataProvider
extension automatically.
#597
Conversation
…n automatically.
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.
Self-review
{ | ||
options.Apply(builder); | ||
builder.AddHocon(DistributedData.DistributedData.DefaultConfig(), HoconAddMode.Append); | ||
builder.WithExtension<DistributedDataProvider>(); |
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.
Actual fix, add the extension to the auto-start list.
var settings = ReplicatorSettings.Create(Sys); | ||
var coordinatorName = settings.RestartReplicatorOnFailure ? $"{ReplicatorName}Supervisor" : ReplicatorName; | ||
|
||
var actorSelection = Sys.ActorSelection(new RootActorPath(cluster.SelfAddress) / "user" / coordinatorName); |
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.
The replicator is started under user guardian, is this correct?
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.
It really should be under the /system
guardian but if we're stuck with /user
for now then that's a separate bug that'll need fixing.
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.
Agreed, this is an Akka core issue, not Akka.Hosting
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.
Could you please file a bug for that?
That might also be an ugly one to chase down if replicators are all currently hard-coded to look for /user/
paths for chatting. Might have to move that to a v1.6 issue if it ends up being a breaking behavioral change.
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.
Issue to track this created here: akkadotnet/akka.net#7606
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 but minor nitpicks
var settings = ReplicatorSettings.Create(Sys); | ||
var coordinatorName = settings.RestartReplicatorOnFailure ? $"{ReplicatorName}Supervisor" : ReplicatorName; | ||
|
||
var actorSelection = Sys.ActorSelection(new RootActorPath(cluster.SelfAddress) / "user" / coordinatorName); |
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.
It really should be under the /system
guardian but if we're stuck with /user
for now then that's a separate bug that'll need fixing.
src/Akka.Cluster.Hosting.Tests/ClusterShardingDistributedDataSpecs.cs
Outdated
Show resolved
Hide resolved
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
Fixes #594
Changes
DistributedDataProvider
to auto-start extension list inside theWithDistributedData()
extension method.Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):