Set up options and track changes in management SDK. #1081

Y-Sindo · 2020-10-27T06:30:24Z

Summary of the changes

Move options-related class to directory "Configuration"
set up ServiceManagerContext to track ServiceManagerOptions, set up ServiceOptions to track ServiceManagerContext
e2e tests passed.

Configuration Methods

DI style

Method1: use configuration instance such as JSON file and so on. It's OK that no configuration file is found.

      services.AddSignalRServiceManager();

{
    "Azure": {
        "SignalR": {
            "ApplicationName": null,
            "ConnectionCount": 1,
            "ConnectionString": "",
            "Proxy": null,
            "ServiceEndpoint": null,
            "ServiceTransportType": 0
        }
    }
}

Method2: use delegate and configuration file.

    services.AddSignalRServiceManager(o=>{
         o.ConnectionString="";
    });

`ServiceManagerBuilder`

    var manager = new ServiceManagerBuilder()
        .WithOptions(o=>o.ServiceTransportType=ServiceTransportType.Persistent)
        .WithConfiguration(new ConfigurationBuilder().AddJsonFile("config.json", reloadOnChange: true).Build())
        .Build();

src/Microsoft.Azure.SignalR.Management/Configuration/ServiceManagerContextSetup.cs

src/Microsoft.Azure.SignalR.Management/ServiceManagerBuilder.cs

src/Microsoft.Azure.SignalR.Management/Configuration/ServiceManagerContextSetup.cs

src/Microsoft.Azure.SignalR.Management/DependencyInjectionExtensions.cs

src/Microsoft.Azure.SignalR.Management/ServiceManagerBuilder.cs

src/Microsoft.Azure.SignalR.Management/Microsoft.Azure.SignalR.Management.csproj

src/Microsoft.Azure.SignalR.Management/ServiceManagerBuilder.cs

src/Microsoft.Azure.SignalR.Management/DependencyInjectionExtensions.cs

src/Microsoft.Azure.SignalR.Management/ServiceManagerBuilder.cs

src/Microsoft.Azure.SignalR.Management/Configuration/CascadeOptionsSetup.cs

src/Microsoft.Azure.SignalR.Management/Configuration/ServiceOptionsSetup.cs

src/Microsoft.Azure.SignalR.Management/DependencyInjectionExtensions.cs

test/Microsoft.Azure.SignalR.Management.Tests/DiExtensionFacts.cs

…erviceManagerOptions is configured.

…, change hot reload unit tests

src/Microsoft.Azure.SignalR.Management/ServiceManagerBuilder.cs

vicancy · 2020-11-03T05:39:03Z

src/Microsoft.Azure.SignalR.Management/ServiceManagerBuilder.cs

-            _options.ValidateOptions();
-
+            _services.AddSignalRServiceManagerCore();
+            _serviceProvider = _services.BuildServiceProvider();


what if Build is called twice? the first _serviceProvider never dispose?

Check added, throw InvalidOperationException() if called twice.

src/Microsoft.Azure.SignalR.Management/ServiceManagerBuilder.cs

vicancy · 2020-11-03T05:46:29Z

src/Microsoft.Azure.SignalR.Common/Constants.cs

@@ -9,6 +9,7 @@ internal static class Constants
    {
        public static class Keys
        {
+            public const string ServiceManagerOptionsSectionKey = "Azure:SignalR";


use a more common name if it is in Common project, or you can move the const to the Management project

Move the key to ServiceManagerOptions as a const string named Section.

src/Microsoft.Azure.SignalR.Management/Configuration/ServiceManagerContextSetup.cs

src/Microsoft.Azure.SignalR.Management/Configuration/ServiceOptionsSetup.cs

src/Microsoft.Azure.SignalR.Management/ServiceManagerBuilder.cs

…ad test;

…s; try to resolve conflict of config file name

vicancy · 2020-11-04T02:59:06Z

src/Microsoft.Azure.SignalR.Management/Configuration/CascadeOptionsSetup.cs

+    internal abstract class CascadeOptionsSetup<TargetOptions> : IConfigureOptions<TargetOptions>, IOptionsChangeTokenSource<TargetOptions>
+        where TargetOptions : class
+    {
+        private readonly ServiceManagerOptions _initialSource;


what needs to be hot-reloaded is the multiple endpoint configure? So is it that we only need to track ServiceManagerOptions => ServiceOptions change? Does it work if ServiceManagerContext inherits ServiceOptions ?

Yes, good idea.

vicancy · 2020-11-04T03:00:00Z

src/Microsoft.Azure.SignalR.Management/Configuration/ServiceManagerOptions.cs

+        /// <summary>
+        /// The section name used to get <see cref="ServiceManagerOptions"/> value from <see cref="IConfiguration.GetSection(string)"/>
+        /// </summary>
+        public const string Section = "Azure:SignalR";


intended to expose it to customer?

Yes, I think customer can get the right position of configuration from Section.

vicancy · 2020-11-04T03:03:12Z

src/Microsoft.Azure.SignalR.Management/ServiceManagerBuilder.cs

-            _options.ValidateOptions();
+            if (ServiceProvider != null)
+            {
+                throw new InvalidOperationException($"Mulitple invocation of the method is not allowed.");


why we have such restriction is because we want UT to test ServiceProvider? otherwise ServiceProvider can be disposed inside Build()?

Should the following case be a valid one?

var manager1 = builder.WithOptions(o=>o.ConnectionString="one").Build(); var manager2 = builder.WithOptions(o=>o.ConnectionString="another").Build();

If ServiceProvider is disposed, then all the services built by service provider are disposed. We should only dispose ServiceProvider only when we don't need ServiceManager anymore.

Now ServiceProvider is disposed from ServiceManager

…ken : ServiceOptions => ServiceManagerContext => ServiceManagerOptions

… for ServiceManagerBuilder.

… service collection.

vicancy · 2020-11-05T05:58:25Z

src/Microsoft.Azure.SignalR.Management/Configuration/ServiceManagerOptionsSetup.cs

+        {
+            if (_configuration != null)
+            {
+                _configuration.GetSection(Constants.Keys.AzureSignalRSectionKey).Bind(options);


I think adding services.Configure<ServiceManagerOptions>(Configuration.GetSection(Constants.Keys.AzureSignalRSectionKey)); is enough, we don't need this setup class as we don't have additional setups? Emulator uses such configure and hotreload works, see https://github.com/Azure/azure-signalr/blob/dev/src/Microsoft.Azure.SignalR.Emulator/Startup.cs#L45

In here, https://github.com/Azure/azure-signalr/blob/dev/src/Microsoft.Azure.SignalR.Emulator/Startup.cs#L45, the Configuration variable is actually an IConfiguration type variable. However, in the extension methods of ServiceCollection, we don't have IConfiguration. Do you think we should add it as a parameter?

vicancy · 2020-11-05T06:06:17Z

src/Microsoft.Azure.SignalR.Management/DependencyInjectionExtensions.cs

+
+        private static IServiceCollection TrySetProductInfo(this IServiceCollection services)
+        {
+            var assembly = Assembly.GetExecutingAssembly();


So this anyway executes even if ProductInfo is already set?

Yes, but if ProductInfo is not null, then the value would not be overwritten.

vicancy · 2020-11-05T06:09:24Z

src/Microsoft.Azure.SignalR.Management/ServiceManager.cs


-        internal ServiceManager(ServiceManagerContext context, RestClientFactory restClientFactory)
+        //in builder mode, serviceProvider is not null and should be disposed.
+        internal ServiceManager(ServiceManagerContext context, RestClientFactory restClientFactory, ServiceProvider serviceProvider = null)


when would ServiceProvider be null?

Yes, some mistakes here . Changed it to IServiceProvider from ServiceProvider and make it required .

vicancy · 2020-11-05T06:14:05Z

src/Microsoft.Azure.SignalR.Management/Configuration/CascadeOptionsSetup.cs

+        where TargetOptions : class
+    {
+        private ConfigurationReloadToken _changeToken;
+        private IDisposable _registration;


vicancy · 2020-11-05T06:16:38Z

src/Microsoft.Azure.SignalR.Management/Configuration/CascadeOptionsSetup.cs

+
+        public string Name => Options.DefaultName;
+
+        public void Configure(TargetOptions target) => Convert(target, _sourceOptions);


is using sourceMonitor.CurrentValue better?

vicancy · 2020-11-05T06:22:46Z

src/Microsoft.Azure.SignalR.Management/ServiceManager.cs

@@ -141,6 +144,14 @@ public async Task<IServiceHubContext> CreateHubContextAsync(string hubName, ILog
            }
        }

+        public void Dispose()
+        {
+            if (_context.DisposeServiceProvider)


is passing some ownServiceProvider parameter in .ctor doable?

… remove null defalut value.

vicancy · 2020-11-09T05:47:59Z

src/Microsoft.Azure.SignalR.Management/ServiceManagerBuilder.cs

+            _services.AddSignalRServiceManager();
+            _services.Configure<ServiceManagerContext>(c => c.DisposeServiceProvider = true);
+            var serviceProvider = _services.BuildServiceProvider();
+            var context = serviceProvider.GetRequiredService<IOptions<ServiceManagerContext>>().Value;
            var productInfo = ProductInfo.GetProductInfo(_assembly);


read from context?

…gnalr into chained-options

Y-Sindo requested review from wanlwanl and JialinXin October 27, 2020 06:30

JialinXin reviewed Oct 27, 2020

View reviewed changes

src/Microsoft.Azure.SignalR.Management/Configuration/ServiceManagerContextSetup.cs Outdated Show resolved Hide resolved

JialinXin reviewed Oct 27, 2020

View reviewed changes

src/Microsoft.Azure.SignalR.Management/ServiceManagerBuilder.cs Show resolved Hide resolved

JialinXin approved these changes Oct 27, 2020

View reviewed changes

vicancy reviewed Oct 28, 2020

View reviewed changes

Y-Sindo force-pushed the chained-options branch from 7c4dbf7 to e889b03 Compare October 30, 2020 06:22

JialinXin reviewed Nov 2, 2020

View reviewed changes

src/Microsoft.Azure.SignalR.Management/Microsoft.Azure.SignalR.Management.csproj Outdated Show resolved Hide resolved

vicancy reviewed Nov 2, 2020

View reviewed changes

Y-Sindo force-pushed the chained-options branch from 2802b44 to a9b7d1b Compare November 2, 2020 07:47

vicancy reviewed Nov 2, 2020

View reviewed changes

Y-Sindo added 17 commits November 3, 2020 11:38

set up ServiceManagerContext and ServiceOptions; track changes

f1d6d2c

fix error in ServiceManagerBuilder as a temporary solution

ef79564

test for options changes detected

04d9b10

make sure product info has default value

1840a4d

fix default product info error

8cb960c

remove limitation that AddSignalRServiceManager must be added after S…

fa833d2

…erviceManagerOptions is configured.

offer choices to configure with configration instance or action

f9b358e

fix

c4f2030

fix

32636be

fix

c83f4c9

Enable service builder to build dynamically configurable service manager

17b6585

add config hot reload to ServiceManagerBuilder, fix productInfo error…

52dc94b

…, change hot reload unit tests

Update DiExtensionFacts.cs

5315a43

fix

d3a3b68

rename & skip fail test

1399157

fix

d32b810

fix

80874da

Y-Sindo force-pushed the chained-options branch from 177ce85 to 80874da Compare November 3, 2020 03:42

fix

6786043

merge ServiceManagerOptions section key into one

3d6a3ec

vicancy reviewed Nov 3, 2020

View reviewed changes

Y-Sindo added 3 commits November 3, 2020 15:43

rename service provider

37947ab

tracks change directly from IConfiguration; Add InMemoryConfigHotRelo…

e3edf86

…ad test;

add memory config hot reload test to DependencyInjectionExtensionFact…

9a5c796

…s; try to resolve conflict of config file name

vicancy reviewed Nov 4, 2020

View reviewed changes

Y-Sindo added 5 commits November 4, 2020 19:34

Implement options changes tracking chain with self-creating IChangeTo…

a9a6153

…ken : ServiceOptions => ServiceManagerContext => ServiceManagerOptions

dispose serviceProvider from serviceManager; delete unnecessary tests…

f6b7d91

… for ServiceManagerBuilder.

fix

2532535

fix

514355a

do not dispose ServiceProvider if ServiceManager is built from global…

13ec165

… service collection.

vicancy reviewed Nov 5, 2020

View reviewed changes

Y-Sindo added 2 commits November 5, 2020 16:20

change ServiceProvider to IServiceProvider in ServiceManager ctor and…

35c083e

… remove null defalut value.

fix

4b73ac4

vicancy approved these changes Nov 9, 2020

View reviewed changes

Y-Sindo added 2 commits November 9, 2020 14:33

read from context

903f1c2

Merge branch 'chained-options' of https://github.com/Y-Sindo/azure-si…

383ad4b

…gnalr into chained-options

Y-Sindo merged commit a602e5c into Azure:dev Nov 9, 2020

Y-Sindo deleted the chained-options branch November 9, 2020 06:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set up options and track changes in management SDK. #1081

Set up options and track changes in management SDK. #1081

Y-Sindo commented Oct 27, 2020 •

edited

Loading

vicancy Nov 3, 2020

Y-Sindo Nov 3, 2020

vicancy Nov 3, 2020

Y-Sindo Nov 3, 2020

vicancy Nov 4, 2020

Y-Sindo Nov 4, 2020

vicancy Nov 4, 2020

Y-Sindo Nov 4, 2020

vicancy Nov 4, 2020

Y-Sindo Nov 4, 2020

Y-Sindo Nov 4, 2020

vicancy Nov 5, 2020

Y-Sindo Nov 5, 2020

vicancy Nov 5, 2020

Y-Sindo Nov 5, 2020

vicancy Nov 5, 2020

Y-Sindo Nov 5, 2020

vicancy Nov 5, 2020

Y-Sindo Nov 5, 2020

vicancy Nov 5, 2020

Y-Sindo Nov 5, 2020

vicancy Nov 5, 2020

vicancy Nov 9, 2020

Y-Sindo Nov 9, 2020


		public string Name => Options.DefaultName;

		public void Configure(TargetOptions target) => Convert(target, _sourceOptions);

Set up options and track changes in management SDK. #1081

Set up options and track changes in management SDK. #1081

Conversation

Y-Sindo commented Oct 27, 2020 • edited Loading

Summary of the changes

Configuration Methods

DI style

Method1: use configuration instance such as JSON file and so on. It's OK that no configuration file is found.

Method2: use delegate and configuration file.

ServiceManagerBuilder

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Y-Sindo commented Oct 27, 2020 •

edited

Loading

`ServiceManagerBuilder`