Expand Cluster.Hosting options #149
Conversation
| /// </summary> | ||
| public string[] Roles { get; set; } | ||
| public Role[]? Roles { get; set; } |
There was a problem hiding this comment.
Don't like the Role type - adds more friction to the configuration surface in order to expose a feature barely anybody uses. Users can configure this in HOCON if they really want it - please remove it from the UI surface area entirely and go back to using simple strings.
| /// If populated, the akka.cluster.seed-nodes that will be used. | ||
| /// If populated, the akka.cluster.seed-nodes that will be used. | ||
| /// </summary> | ||
| public Address[]? SeedNodes { get; set; } |
There was a problem hiding this comment.
So here's a goal worth considering with the ClusterOptions class - should this be parseable from an appsettings.json file / Environment Variable? If so, maybe we should support string here instead and just handle the parsing on the back-end. I'm leaning towards doing that for 1.0.
There was a problem hiding this comment.
Yeah, let's change this to a string type
There was a problem hiding this comment.
Here's what we have to do in the current stable version of Akka.Hosting:
var akkaSection = context.Configuration.GetSection("Akka");
// maps to environment variable Akka__ClusterIp
var hostName = akkaSection.GetValue<string>("ClusterIp", "localhost");
// maps to environment variable Akka__ClusterPort
var port = akkaSection.GetValue<int>("ClusterPort", 0);
var seeds = akkaSection.GetValue<string[]>("ClusterSeeds", new[] { "akka.tcp://SqlSharding@localhost:7918" })
.Select(Address.Parse)
.ToArray();It would be amazing if we could just do this instead (and if we decide to go down this route, we should write a unit test that looks like this):
appsettings.json
{
"Logging": {
"LogLevel": {
"Default": "Debug",
"System": "Information",
"Microsoft": "Information"
}
},
"Akka": {
"RemoteOptions":{
"Hostname": "localhost",
"Port": 8081,
"PublicHostname": "localhost",
"PublicPort": 8081
},
"ClusterOptions": {
"SeedNodes": "akka.tcp://ClusterSystem@localhost:8081",
"Roles": ["backend"]
}
}
}program.cs
var akkaSection = context.Configuration.GetSection("Akka");
var clusterConfig = akkaSection.Get<ClusterOptions>(); // ideally, this should workWe've not really stated this as an explicit goal for the project before, but in the context of our "pit of success" mission for this library I think it's probably worth doing before we ship 1.0. What do you think?
| /// is completed when the singleton actor is terminated. Note that <see cref="PoisonPill"/> is a | ||
| /// perfectly fine <see cref="TerminationMessage"/> if you only need to stop the actor. | ||
| /// </summary> | ||
| public object? TerminationMessage { get; set; } |
| } | ||
|
|
||
| public sealed class ShardOptions | ||
| { | ||
| /// <summary> |
There was a problem hiding this comment.
All of the ShardOptions changes LGTM
| /// </summary> | ||
| public Address[] SeedNodes { get; set; } | ||
| public Dictionary<string, int>? MinimumNumberOfMembersPerRole { get; set; } |
There was a problem hiding this comment.
This is the line I ment.
Would this be a better compromise to the Role class?
We're not forcing the user to provide any values and still keeps the class as simple as possible.
| /// </summary> | ||
| public Address[] SeedNodes { get; set; } | ||
| public Dictionary<string, int>? MinimumNumberOfMembersPerRole { get; set; } |
# Conflicts: # src/Akka.Cluster.Hosting/AkkaClusterHostingExtensions.cs
Part of #142
Changes