Storage providers instantiation by jdom · Pull Request #1 · jason-bragg/Oxygen

jdom · 2016-09-24T06:08:07Z

This PR is just work in progress before I go to sleep, but it is basically how I envision the providers instantiation (as we know they are not singleton services).
This is modeled from how AspNet allows optional features.
The important thing is how the Startup class looks like, and how the configuration is done via extension methods on IServiceCollection and ISiloBuilder (the latter I added emulating the IApplicationBuilder from Asp.NET)

This does not yet use IOptions<T> to configure the storage providers' specific info, but that's coming later if you feel the approach is good.

For now I put everything in the abstractions project, but in theory they should be moved to different pieces (I added code comments specifying where they should live).

This does not yet use IOptions to configure the storage providers' specific info, but that's coming later

jason-bragg · 2016-09-24T07:11:41Z

Thanks for tinkering with me!
Suspected I'd be alone in here..

jason-bragg · 2016-09-24T07:00:42Z

Orleans/src/Microsoft.Orleans.Host.Abstractions/ISiloBuilder.cs

+
+namespace Microsoft.Orleans.Host.Abstractions
+{
+    public interface ISiloBuilder


Was planning to put silo builder in Microsoft.Orleans.Silo.Abstractions.

A goal of this design is logical separation of actor model, i.e silo and service framework, i.e host. Silo should be able to be run in any service framework. Host is our framework as the silo is not much value without a framework to run it in.

jason-bragg · 2016-09-24T07:03:27Z

Orleans/src/Microsoft.Orleans.Host.Abstractions/IStartup.cs

    public interface IStartup
    {
-        IEnumerable<Type> Members { get; }
+        IEnumerable<Type> Members { get; } // not sure I understand why this?


This is for the purpose of constructing any systems the host contains. Like a silo, in this way, a silo is just another pluggable component in the service framework. Provider are too. IMO, this silo should not concern itself with providers, they are a framework detail.

jason-bragg · 2016-09-24T07:06:47Z

Orleans/Test/TinkerHostTests/TinkerHostLifecycleTests.cs

            {
                logger = loggerFactory.CreateLogger<Thingy>();
-                lifecycle.Subscribe(this);
+                lifecycle.Subscribe(this); //<-- passing this before finishing constructing is dangerous (although practical)


yeah, I was being lazy. There are extension functions to connect callbacks. I'll put out a PR to illustrate.

jason-bragg · 2016-09-24T07:08:14Z

Orleans/src/Microsoft.Orleans.Host.Abstractions/AzureStorageProvider.cs

+namespace Microsoft.Orleans.Host.Abstractions
+{
+    // this would go into the azure storage providers DLL
+    public static class AzureStorageProviderExtensions


extensions should go in an {namespace}.Extensions namespace according to framework standards. Users should have the option to opt in.

Yup, as I mentioned in the description, I just put everything in the same project for now and didn't even consider the namespace, just showing what the programing Api would look like (mainly startup class) . Also, some of these new files mention that they should go in different Dlls that you only pull in if you use them

jason-bragg · 2016-09-24T07:09:46Z

Orleans/src/Microsoft.Orleans.Host.Abstractions/StorageProvidersFeature.cs

+    // this would go into the (optional) storage providers DLL.
+    public interface IStorageProvidersFactory
+    {
+        ISiloBuilder Silo { get; }


Why should as silo builder be part of this interface..?

Because it allows you to access runtime service that can be used by it or passed along for concrete provider factories to use, as in this azure storage example

jason-bragg · 2016-09-24T07:10:38Z

Orleans/src/Microsoft.Orleans.Host.Abstractions/StorageProvidersFeature.cs

+namespace Microsoft.Orleans.Host.Abstractions
+{
+    // this would go into the (optional) storage providers DLL.
+    public interface IStorageProvidersFactory


This interface doesn't have any characteristics of a factory..

Yup, I agree, remnants of previous prototyping. This is more an IStorageProviderManager

or IStorageProvidersBuilder actually, and then it would be more obvious why it has an ISiloBuilder too.

jdom · 2016-09-24T08:05:38Z

Orleans/src/Microsoft.Orleans.Host.Abstractions/AzureStorageProvider.cs

+namespace Microsoft.Orleans.Host.Abstractions
+{
+    // this would go into the azure storage providers DLL
+    public static class AzureStorageProviderExtensions


Yup, as I mentioned in the description, I just put everything in the same project for now and didn't even consider the namespace, just showing what the programing Api would look like (mainly startup class) . Also, some of these new files mention that they should go in different Dlls that you only pull in if you use them

jdom · 2016-09-24T08:07:52Z

Orleans/src/Microsoft.Orleans.Host.Abstractions/StorageProvidersFeature.cs

+namespace Microsoft.Orleans.Host.Abstractions
+{
+    // this would go into the (optional) storage providers DLL.
+    public interface IStorageProvidersFactory


Yup, I agree, remnants of previous prototyping. This is more an IStorageProviderManager

jdom · 2016-09-24T08:09:33Z

Orleans/src/Microsoft.Orleans.Host.Abstractions/StorageProvidersFeature.cs

+    // this would go into the (optional) storage providers DLL.
+    public interface IStorageProvidersFactory
+    {
+        ISiloBuilder Silo { get; }


Because it allows you to access runtime service that can be used by it or passed along for concrete provider factories to use, as in this azure storage example

jdom · 2016-09-24T08:13:17Z

Orleans/src/Microsoft.Orleans.Host.Abstractions/AzureStorageProvider.cs

+    {
+        public static void AddAzureStorage(this IStorageProvidersFactory factory, string providerName, string connectionString)
+        {
+            var concreteFactory = ActivatorUtilities.CreateInstance<AzureStorageProviderFactory>(factory.Silo.ApplicationServices);


I believe that the AzureStorageProviderFactory might not be required if I use the right overload for ActivatorUtilities.CreateInstance, since the factory is there mainly to forward injected service dependencies and separate them from instance specific data/config

jason-bragg · 2016-09-25T02:38:02Z

So I updated tree with a second prototype pass on the provider model.
Still not convinced it's right, but trying to explore a couple ideas.

Separation of framework from Virtual Actor container (Silo)
Providers add capabilities to the framework. If the framework itself is extensible, providers are not necessary, so the question is how to make the framework extensible enough to support the behaviors of providers. Once we figure out how the silo works within the framework, we can access these extensibility points from within the silo. This changes the question from how do we support providers to what are provider behaviors and how do we support these behaviors within the framework.

Providers are not Services
Providers can't properly be considered services from the DI sense, as they have per instance initialization. Grains also have this problem, but for now I'm focusing on the general problem: How do we support per instance initialization.

Current Prototype
In the current prototype, I'm using a factory pattern with an optional configuration (after creation) step, to support per instance initialization. For providers, I use the factory pattern to create a provider via DI, so it can be constructed with dependencies, then configure it after construction. The provider itself, gets things it needs like lifecycle and logger factory at construction time, is configured, and is initialized when appropriate in the lifecycle.
A provider group serves as a singleton in the DI system as a means of accessing specific providers. At construction time, it constructs all configured providers, but doesn't know anything about the provider’s construction, configuration, or initialization.
Grains that need a provider to have the provider group injected at construction time, and then ask the provider group for the provider by name.

In this pattern, the component that knows most about the provider is the provider itself and its configuration step. Everything else in the system knows only what it needs. Provider group just knows that it exists and the caller just knows the client behavior.

While this design achieves most of my goals, there is a problem with configuration, because the behavior is supported by a collection of classes that all need to setup. This degree of complexity can't easily be hidden by a per provider extension function like AddStorageProvider, so simplifying the configuration/setup is something that will need more thought.

jason-bragg · 2016-09-25T06:09:54Z

Took another pass on configuration. Much nicer now.
Patter also allows lookup of provider by any key. Sample uses string and Guid.
Configuration code below.

            services.AddSingleton<ISiloRuntime, DummySiloRuntime>()
                    .AddTransient<SomePersistentGrain>()
                    .AddProviderGroup<string,IStorageProvider>()
                        .AddAzureStorageProvider("Azure", "secret")
                        .AddAzureStorageProvider("Azure2", "secret2")
                    .Build()
                    .AddProviderGroup<Guid, IStorageProvider>()
                        .AddAzureStorageProvider(Guid.NewGuid(), "secret3")
                        .AddAzureStorageProvider(Guid.NewGuid(), "secret4")
                        .AddAzureStorageProvider(Guid.NewGuid(), "secret5")
                        .AddAzureStorageProvider(Guid.NewGuid(), "secret6")
                    .Build();

jason-bragg · 2016-09-25T09:24:34Z

Tinkered with silo and multi-silo hosts using the provider model.

Since service provider supports singletons and provider model (probably need to rename it) supports unique instances, we can create, configure, initialize, run, and stop one or more silos in the host, by simply changing the startup class.

Members management needs to change though, I think. Members, like services, need to be modifiable in a better way. Have an idea how to do this, but need to think on it more.

xiazen

I probably should just use GitHub comment , instead of starting a review, since this is a prototype PR.

But hey I'm playing with the new GitHub feature!

xiazen · 2016-09-26T06:02:52Z

Orleans/src/Microsoft.Orleans.Host.Abstractions/AzureStorageProvider.cs

+    {
+        public static void AddAzureStorage(this IStorageProvidersBuilder factory, string providerName, string connectionString)
+        {
+            var concreteFactory = ActivatorUtilities.CreateInstance<AzureStorageProviderFactory>(factory.Silo.ApplicationServices);


feels like concreteFactory can be reused for creating other AzureStorageProvider, which should be a singleton. maybe the method signature should be
public static void AddAzureStorage(this IStorageProvidersBuilder factoryBuilder, AzureStorageProviderFactory factory, string providerName, string connectionString)

xiazen · 2016-09-26T06:08:34Z

Orleans/src/Microsoft.Orleans.Host.Abstractions/AzureStorageProvider.cs

+    // this would go into the azure storage providers DLL
+    public static class AzureStorageProviderExtensions
+    {
+        public static void AddAzureStorage(this IStorageProvidersBuilder factory, string providerName, string connectionString)


Why isn't this method member of AzureStorageProviderFactory?

If you worry about AzureStorageProviderFactory'saccessability as an internal class, maybe we can use an interface as a medium to publicize AddAzureStorage method:
interface IAzureStorageProviderFactory: { public AddAzureStorage(...) }

because this extendes the IStorageProvidersBuilder interface, not the concrete azure factory. In fact, as I mentioned in a comment, the factory itself might even go away, it's just an implementation detail.

xiazen · 2016-09-26T06:11:20Z

Orleans/src/Microsoft.Orleans.Host.Abstractions/StorageProvidersFeature.cs

+    // this would go into the (optional) storage providers DLL.
+    public interface IStorageProvidersBuilder
+    {
+        ISiloBuilder Silo { get; }


not sure why IStorageProviderBuildershould have a member of ISiloBuilder. Feels to me IStorageProviderBuildershould be member of ISiloBuilder, so it will build StorageProviders based on silo's lifecycle event. Not the other way around.

this way storage providers are an opt-in framework feature that do not require to be bundled in the virtual actors core. You decide whether you want to use storage providers explicitly, and since it's entirely opt-in, you might decide to use an alternative storage providers model entirely, because the framework would be flexible enough to support it.

xiazen · 2016-09-26T06:15:21Z

Orleans/src/Microsoft.Orleans.Host.Abstractions/StorageProvidersFeature.cs

+        public static void ConfigureStorageProviders(this ISiloBuilder silo, Action<IStorageProvidersBuilder> configureMethod)
+        {
+            var factory = silo.ApplicationServices.GetRequiredService<IStorageProvidersBuilder>();
+            configureMethod(factory);


Ah I see why you give IStorageProviderBuilder a SiloBuilder memeber. You want to get the registered IStorageProvidersBuilderfrom it. But this is the sevice locator pattern we want to avoid. Well I think you can do ConfigureStorageProviders(IStorageProviderBuilder builder, Action<IStorageProvidersBuilder> configureMethod) and get IStorageProvidersBuilder before calling this method

not sure I follow. This is not client code. The programming API for the user does not use the service locator pattern, it's all abstracted away by these configuration methods, in the same way as Asp.NET hides these too. If we change the signature to what you suggested, it means the user will be in charge of resolving the IStorageProvidersBuilder himself and then pass it in (probably by putting the burden on him to use the service locator pattern

xiazen · 2016-09-26T06:18:06Z

Orleans/src/Microsoft.Orleans.Host.Abstractions/StorageProvidersFeature.cs

+    }
+
+    public interface IStorageProvider
+    {


I think this can inherite from ILifecycleObserver . So that all IStorageProvider which share similar life cycle style/pattern can be managed in the same LifeCycle class.

xiazen · 2016-09-26T07:24:09Z

Per @jason-bragg comment. Not sure why I cannot just replied to @jason-bragg comment

Minor:

Grains that need a provider to have the provider group injected at construction time, and then ask the provider group for the provider by name.

If Grain takes provider group as a param in contruction time, this is service locator pattern again, which I think we should really avoid. Maybe we can have a

Class GrainFactory:
{
   GrainFactory(ProviderGroup group);
   Create(Grain grain); // Grain will give you a list of Provider names it needs. And GrainFactory will find those providers in ProviderGroup and create the Grain. 
}

Since ProviderGroup is really just a dependency of GrainFactory, not every single Grain.

Confusion:

While this design achieves most of my goals, there is a problem with configuration, because the behavior is supported by a collection of classes that all need to setup. This degree of complexity can't easily be hidden by a per provider extension function like AddStorageProvider, so simplifying the configuration/setup is something that will need more thought.

Why isn't Providers managed by LifeCycle class as ILifecycleObserver? So each type of Provider can have its own OnStartUp method or OnConfigure method which makes the lifecycle management much more flexible.
Something like this:

class AzureProvider : ILifecycleObserver
{
  AzureProvider(Config config);
 void OnInitialize(Config config) {// AzureProvider configure method}
 void OnStartUp() {//AzureProvider start up configure}
 void OnStop() {...}
}

class AWSProvider: ILifecycleObserver
{
 AWSProvider(Config config)
void OnInitialize() {// AWSProvider configure method}
 void OnStartUp() {//AWSProvider start up configure}
 void OnStop() {...}

}

configure providers :

class ProviderLifeCycle : LifeCycle
{
//inherit from LifeCycle class, just want it to be just managing providers, similar as a ProviderGroup 
}
class StartUp:
{
 public IServiceProvider ConfigureServices(IServiceCollection services)
{
  ProviderLifecycle providersManager = new ProviderLifeCycle();
  providerManager.Subscribe(config1.name, new AzureProvider(config1));  
  providerManager.Subscribe(config2.name, new AzureProvider(config2));
  providerManager.Subscribe(config3.name, new AWSProvider(config3));
  services.addSingleton<ProviderLifeCycle>(providersManager);
   ...
}
}

This strike to me that LifeCycle, or ILifecycleObservershould have a method like GetObserver(Tkey key), so it can get one Observer from LifeCycleobject and Start it or Terminate it or just use it (to stream or store something or resolve dependency for grains), instead of a group termination or a group start up. Just to adds more flexibility.

attilah · 2016-09-26T17:30:50Z

Orleans/src/Microsoft.Orleans.Host.Abstractions/StorageProvidersFeature.cs

+        {
+            Silo = silo;
+        }
+        public ISiloBuilder Silo { get; }


It something is a builder, don't miss the Builder from the name, it can be confusing.

OK. Yeah, I don't understand why in ASP.NET they just call their IApplicationBuilder "app"... Or maybe it was just in param names, but not necessarily properties

jdom added 2 commits September 23, 2016 21:28

comments

70f1857

Example of how to build storage provider instances (WIP).

2258900

This does not yet use IOptions to configure the storage providers' specific info, but that's coming later

jason-bragg reviewed Sep 24, 2016

View reviewed changes

jdom commented Sep 24, 2016

View reviewed changes

Rename to IStorageProvidersBuilder

7a09c3d

xiazen reviewed Sep 26, 2016

View reviewed changes

attilah reviewed Sep 26, 2016

View reviewed changes

Conversation

jdom commented Sep 24, 2016

Uh oh!

jason-bragg commented Sep 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdom Sep 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdom Sep 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jason-bragg commented Sep 25, 2016

Uh oh!

jason-bragg commented Sep 25, 2016

Uh oh!

jason-bragg commented Sep 25, 2016

Uh oh!

xiazen left a comment

Choose a reason for hiding this comment

Uh oh!

xiazen Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiazen Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiazen commented Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jdom Sep 24, 2016 •

edited

Loading

jdom Sep 24, 2016 •

edited

Loading

xiazen Sep 26, 2016 •

edited

Loading

xiazen Sep 26, 2016 •

edited

Loading

xiazen commented Sep 26, 2016 •

edited

Loading