Update Discovery.Azure to support multi-config #2595
Conversation
| @@ -66,6 +68,7 @@ public void SettingsWithOverrideTest() | |||
| var uri = new Uri("https://whatever.com"); | |||
| var credential = new DefaultAzureCredential(); | |||
| var settings = AzureDiscoverySettings.Empty | |||
| .WithReadOnlyMode(true) | |||
There was a problem hiding this comment.
This makes sense - there haven't been any use cases before now where we needed read-only.
| failure: e => new DiscoveryStopFailed(sender, e)); | ||
| Become(Stopping); | ||
|
|
||
| if (!_readOnly) |
| @@ -123,11 +128,17 @@ public static class AkkaHostingExtensions | |||
| TableClientOptions? tableClientOptions = null, | |||
| string? serviceName = null, | |||
| string? publicHostname = null, | |||
| int? publicPort = null) | |||
| int? publicPort = null, | |||
| string discoveryId = "azure", | |||
There was a problem hiding this comment.
What does discoveryId mean?
There was a problem hiding this comment.
Discovery Id is the HOCON configuration key the final HOCON settings will live in. For example, "azure" the default Id will be placed in "akka.discovery.azure", just like how Akka.Persistence journal Id works. This will allow it to be addressed and loaded by the Akka.Discovery plugin later on.
|
|
||
| public static class Extensions | ||
| { | ||
| public static Configuration.Config MoveTo(this Configuration.Config config, string path) |
There was a problem hiding this comment.
Can this be made internal?
| int? publicPort = null, | ||
| string discoveryId = "azure", | ||
| bool? readOnly = null, | ||
| bool isDefaultPlugin = true) |
There was a problem hiding this comment.
An idea: what if we just used the service-name to alias the HOCON settings, so there's only one moving thing that needs to be tracked? This way, we don't even need to worry about default plugins et al.
There was a problem hiding this comment.
That works, but that will break previous behavior.
There was a problem hiding this comment.
That would probably be ok as long as we had some way of covering the default case from before
| _system.Settings.InjectTopLevelFallback(DefaultConfig); | ||
| _settings = AzureDiscoverySettings.Create(system); | ||
| var fullConfig = config.WithFallback(DefaultConfig.GetConfig("akka.discovery.azure")); | ||
| _settings = AzureDiscoverySettings.Create(system, fullConfig); | ||
|
|
||
| var setup = _system.Settings.Setup.Get<AzureDiscoverySetup>(); |
There was a problem hiding this comment.
@Aaronontheweb This line is currently problematic, a single registered AzureDiscoverySetup instance can pollute every instance of this ServiceDiscovery instance. Would need to look at how Akka.Persistence handles this.
There was a problem hiding this comment.
Fixed this using a hack by embedding the discovery id inside the Akka.Hosting generated HOCON config. We can map/corellate which config goes to which setup class this way.
| // We're going to cheat and embed the config path/discovery id here | ||
| // because we need to correlate the setup files with the config | ||
| // during run-time | ||
| sb.AppendLine($"discovery-id = {ConfigPath}"); |
|
|
||
| namespace Akka.Discovery.Azure; | ||
|
|
||
| internal sealed class AzureDiscoveryMultiSetup : Setup |
There was a problem hiding this comment.
I assume this is internal because it's only accessible via Akka.Hosting
| # If set to true, the extension will not participate in updating the Azure table. | ||
| # Only need to be set to true if the extension is being used by a read only extension | ||
| # such as ClusterClient contact discovery. | ||
| read-only = false |
Changes
akka.discovery.methodvalue