Make StreamQueueBalancer Pluggable #3152

xiazen · 2017-06-21T01:33:47Z

Make StreamQueueBalancer pluggable,
Make it possible for the injected StreamQueueBalancer to be built on top of an async service, such as external storage, or a lease manager.

- Make it possible for the injected StreamQueueBalancer to be built on top of an async service, such as external storage, or a lease manager.

xiazen · 2017-06-21T01:35:26Z

src/Orleans/Streams/PersistentStreams/IStreamQueueBalancer.cs

+        /// Async Initialize logic
+        /// </summary>
+        /// <returns></returns>
+        Task Initialize();


add hock for async initialize logic, such as load queue balancing info from an async storage service, or connect async lease manager for initialize logic.

The same reason for other methods to be async

xiazen · 2017-06-21T01:37:26Z

src/Orleans/Streams/Providers/IStreamProviderRuntime.cs

@@ -99,6 +99,9 @@ public class PersistentStreamProviderConfig
        public const string SILO_MATURITY_PERIOD = "SiloMaturityPeriod";
        public static readonly TimeSpan DEFAULT_SILO_MATURITY_PERIOD = TimeSpan.FromMinutes(2);

+        //optional param. user need to set this when they use Custom StreamQueueBalancer
+        public const string QUEUE_BALANCER_FACTORY_NAME = nameof(QueueBalancerFactoryName);
+        public string QueueBalancerFactoryName { get; set; }


User needs to firstly set QueueBalancerFactoryName and set BalancerType to be CustomBalancer, secondly inject their IStreamQueueBalancerFactory with the set name to DI to be able to use this feature.

xiazen · 2017-06-21T01:39:46Z

test/ServiceBus.Tests/PlugableQueueBalancerTests/LeaseManagerGrain.cs

+namespace Tester.StreamingTests
+{
+    //one lease manager grain per stream provider, so its key is stream provider name
+    public interface ILeaseManagerGrain : IGrainWithStringKey


Just used in tests.
The test is a small probe work to prototype LeaseManager based queue balancer, mainly to make sure it can access most dependency object it needs to access.

xiazen · 2017-06-21T01:46:56Z

test/ServiceBus.Tests/PlugableQueueBalancerTests/LeaseManagerGrain.cs

+        {
+            this.siloStatusOracle = base.ServiceProvider.GetRequiredService<ISiloStatusOracle>();
+            this.clusterConfiguration = base.ServiceProvider.GetRequiredService<ClusterConfiguration>();
+            this.queueMapper = base.ServiceProvider.GetServiceByName<IStreamQueueMapper>(this.GetPrimaryKeyString());


"LeaseManager being grain" has been discussed as a possibility. I want to make sure LeaseManagerGrainBasedQueueBalancer can be an option later.

Just want to bring up the concern that, if we go with "Lease manager being a grain" option, assign it complex dependency will not be intuitive with this PR's interface design.

As an example: I found it difficult for LeaseManagerGrain to access StreamQueueMapper, which intuitively should be its dependency. This is a pretty hacky way to access it. We can think of other more elegant way in the future. But I think it is fine to do it in tests ;)

xiazen · 2017-06-21T01:47:45Z

test/ServiceBus.Tests/PlugableQueueBalancerTests/PluggableQueueBalancerTests.cs

+            protected override TestCluster CreateTestCluster()
+            {
+                var options = new TestClusterOptions(siloCount);
+                ProviderSettings.EventHubPartitionCount = totalQueueCount;


Use EventDataGeneratorStreamProvider mainly because it is a very configurable provider, I can assign any queue count I like through config

Would prefer if these tests where in a test harness so we could run them against other persistent providers.
Initial test should probably use memory stream provider, It allows the same queue flexibility, while also allowing us to do this in a sharable harness.

yeah make sense !

xiazen · 2017-06-22T18:00:49Z

@dotnet-bot test netfx-functional

ReubenBond · 2017-06-22T18:02:27Z

src/Orleans/Streams/PersistentStreams/IStreamQueueBalancer.cs

    {
        /// <summary>
+        /// Async Initialize logic


Something like "Initializes this instance", perhaps

hmm, I'm not sure I follow. What else instance will this be initializing? it is initializing itself. So "this instance" is a bit redundant to me

EDIT: oh you meant the xml comments. I though you meant the method name. yeah we can add that

yep, just the doc comments. There are many Initialize methods in Orleans, so I wanted them to be aligned. Some examples from our code:

/// <summary> /// Initializes the external serializer. Called once when the serialization manager creates /// an instance of this type /// </summary> void Initialize(Logger logger); /// <summary> /// Initializes the client runtime from the standard client configuration file. /// </summary> public static void Initialize() /// <summary> /// Initializes this instance. /// </summary> public void Initialize()

We don't use StyleCop, but standard MS StyleCop rules check that constructor doc comments all follow a pattern like this:

/// <summary> /// Initializes a new instance of the <see cref="AssemblyProcessor"/> class. /// </summary>

ReubenBond · 2017-06-22T20:43:17Z

src/Orleans/Streams/PersistentStreams/StreamQueueBalancerType.cs

+        /// <summary>
+        /// Use user configured custom queue balancer
+        /// </summary>
+        CustomBalancer


It would result in a larger change, but I have strong preference to fetching the balancer from DI and only using this type if no balancer has been added to the container. The NamedServices DI feature can hopefully be used so that different providers can have different balancers.

Check out ReminderTableFactory for an example of how this is done today. It will have to change a bit for named services, but it's registered in Silo as:

services.AddSingleton<ReminderTableFactory>(); services.AddSingleton<IReminderTable>(sp => sp.GetRequiredService<ReminderTableFactory>().Create());

If the user injects their own IReminderTable, then that will be used instead.
Ideally we would only add our default one if they have not added their own - maybe @jdom knows a better way?

@jason-bragg has similar concern on that configuration is a bit redundant. He suggested something which convinced me, and I think you will like it.

He suggested remove this type, and make StreamQueueBalancerType.cs a list of static string, which list the build in queue balancers. so the runtime can resolve the queue balancer using named service. And when user want to inject their own queue balancer, they can configure PersistenStreamProviderConfig.BalancerType to be their custom queue balancer type by giving it a name. And we can remove the other QueueBalancerFactoryName config .

I think it is similar to what you suggested. Hope this make sense, or you will see in my PR update ;)

As to add balancers to DI. I suspect the value of it. Objects are put into DI because it needs to be available in multiple places. But for balancers, it got created once per stream provider and pass to pulling agent manager, and it's done. I don't see where DI comes to play here.

And adding balancer to DI doesn't play well with its life cycle. every stream provider create balancer at the step of initializing pulling agent manager, with a different set of params per stream provider. This is hard to access and differentiate in the step of building DI container

It make sense to me to add BalancerFactory to DI as a named service, because that's the most convenient and familiar pattern for Orleans user to configure a custom service now.

ReubenBond · 2017-06-22T22:05:57Z

src/OrleansRuntime/Streams/QueueBalancer/DeploymentBasedQueueBalancer.cs

            }
        }

-        public bool UnSubscribeToQueueDistributionChangeEvents(IStreamQueueBalanceListener observer)
+        public Task<bool> UnSubscribeToQueueDistributionChangeEvents(IStreamQueueBalanceListener observer)


We should rename this: UnsubscribeFromQueueBalancingEvents and similar for the subscribe method

why do you favor QueueBalancing over QueueDistribution? queue balancing is a verb, and queue distribution is a norm which describe current queue distribution state, right? seems to me , it make more sense to use QueueDistribution

Use queue balancing is a bit confusing. it doesn't indicate what kind of events listener will be receiving. Does it receive only queue distribution change events, or does it receiver balancer state changing also, which listener care less?

ReubenBond · 2017-06-22T22:09:03Z

src/Orleans/Streams/PersistentStreams/IStreamQueueBalancer.cs

@@ -37,7 +42,7 @@ internal interface IStreamQueueBalancer
    /// It should be implemented by components interested in stream queue load balancing.
    /// When change notification is received, listener should request updated list of queues from the queue balancer.
    /// </summary>
-    internal interface IStreamQueueBalanceListener : IAddressable
+    public interface IStreamQueueBalanceListener : IAddressable


Does this need to be IAddressable?

it was IAddressable before. But you are right, this limits listener to be only IAddressable.

I wonder is there historical reason for it @jason-bragg ?

xiazen · 2017-06-23T20:30:26Z

src/Orleans/Streams/PersistentStreams/StreamQueueBalancerType.cs

+    /// <summary>
+    /// Built-in stream queue balancer type which is supported natively in orleans
+    /// </summary>
+    public static class BuiltInStreamQueueBalancerType


Changing the name of this class will make it a breaking change. But I slightly prefer this name, which has clearer indication, especially now we support pluggable queue balancer now. But I will be fine if you want to change it back

let's leave the enum, and have this class expose string for the enums. That way legacy code/tests can still use these enum values.

In the config object we can add a 'string QueueBalancer' property and change the BalancerType to be a passthrough that setts the QueueBalancer'. The property in the property bag would remain "BalancerType"

?? if you are worrying about breaking change, only by changing the name would make this a breaking change. Change it from a enum class to a static class with strings won't make it a breaking change. By breaking change I mean: user will have compiling errors from it. To check this, you can change it from a enum class to a static class and compile your test project, and see if you get any compile errors. Maybe I missed something.

And I don't fully understand the logic behind an extra QueueBalancer setting. To avoid breaking change? Then back to my original question.

I guess my question is, what is your concern? if you are worrying about breaking change, then I should just change the name back, right?

I was concerned about breaking changes. It seems like we could maintain the old enums and add support for strings, but in retrospect, it's probably best to have only one supported way (strings) and eat the cost of a breaking change.

I withdraw my suggestion.

but it is not a breaking change if we change this class's name back. If we want to avoid breaking change, then we need to change the class name back, which is what I try to say.
People use this as BalancerType = StreamQueueBalancerType.WhateverType. after this change, object in both sides of the "=" will become string, so it won't cause compile problems.

xiazen · 2017-06-23T20:34:39Z

src/OrleansRuntime/Streams/QueueBalancer/IStreamQueueBalancerFactory.cs

+namespace Orleans.Streams
+{
+    public interface IStreamQueueBalancerFactory
+    {


still think this should be an interface, instead of a factory delegate...
I think factory as a delegate will be good if it is part of a bigger class's property, it is light weight that way. But that is not true in this PR.
User inject a factory instance, instead of a factory delegate fits more into OOP patterns

xiazen · 2017-06-23T23:00:32Z

updated with fixes for PR feedback

jason-bragg · 2017-06-23T23:09:36Z

src/Orleans/Streams/PersistentStreams/IStreamQueueBalancer.cs

        /// Retrieves the latest queue distribution for this balancer.
        /// </summary>
        /// <returns>Queue allocated to this balancer.</returns>
-        IEnumerable<QueueId> GetMyQueues();
+        Task<IEnumerable<QueueId>> GetMyQueues();


Why did you make these tasks?

I explained it in my comments before: to make it possible for the queue balancer to be built on top of an async service, such as an async lease manager, or a storage service.

Ah, sorry, I missed that. Now I understand.
Ok, so we'd considered this in the original pattern, and chose for this to be synchronous. The reason for this is that this is a push+pull pattern. We notify the IStreamQueueBalanceListeners once we get a change, so we already have the change at that point, so there is no reason to be asynchronous. This is actually important, because the agent is reentrant and giving up it's turn to get the queues could have unexpected side effects. Even the subscribe and unsubscribe calls were expected to be local, because the queue balancer is expected to be per silo, we don't want it to be distributed and asynchronous.

will change it back

Thanks for the heads up

May need to note in the comments, that GetMyQueues, SubscribeToQueueDistributionChangeEvents, and UnSubscribeFromQueueDistributionChangeEvents will likely be called in the IStreamQueueBalanceListener's thread so they need to be thread safe.

jason-bragg · 2017-06-26T17:32:09Z

src/OrleansRuntime/Streams/QueueBalancer/StreamQueueBalancerFactory.cs

@@ -21,7 +22,7 @@ internal class StreamQueueBalancerFactory
        /// <param name="siloMaturityPeriod">Maturity Period of a silo for queue rebalancing purposes</param>
        /// <returns>Constructed stream queue balancer</returns>
        public static IStreamQueueBalancer Create(
-            StreamQueueBalancerType balancerType,
+            string balancerType,
            string strProviderName,
            ISiloStatusOracle siloStatusOracle,


ISiloStatusOracle, ClusterConfiguration, and IStreamProviderRuntime, can be obtained by DI in the factory's constructor. We should probably remove them from the Create call.

not a big fan of service locator pattern. but seems like that's the orleans style now. we use it in grain public API too.....
so remove those and add IServiceProvider param?

My thinking here is to avoid passing anything that can be obtained by DI, passing only the things that the queue balancer manager alone has, like balancer type, provider name, and maturity period. The container can be obtained from DI, right?

yes, but we don't need the container either.

see my latest commit. We decided to shift from "configure balancer through a factory" to "configure balancer through its init method", as a more straight-forward alternative. so this factory interface is removed.
Concern for a keyed factory is, it split configuration into two places: factory class and keyed service config in DI, which IMO is not an very comprehensive pattern to users. And the constraits we put on the interface is not enough to avoid bad user code. It's easy for user to misunderstand this feature as "inject an factory as a singleton and do a case-switch inside its create method to generate the correct balancer"

jason-bragg · 2017-06-28T00:05:54Z

src/Orleans/Streams/Providers/IStreamProviderRuntime.cs

@@ -91,20 +91,25 @@ public class PersistentStreamProviderConfig
        public static readonly TimeSpan DEFAULT_STREAM_INACTIVITY_PERIOD = TimeSpan.FromMinutes(30);

        public const string QUEUE_BALANCER_TYPE = "QueueBalancerType";
-        public const StreamQueueBalancerType DEFAULT_STREAM_QUEUE_BALANCER_TYPE = StreamQueueBalancerType.ConsistentRingBalancer;
+        //default balancer type if ConsistentRingBalancer
+        public static Type DEFAULT_STREAM_QUEUE_BALANCER_TYPE = null;


Don't think we need this.. default would be null anyway, right?

mainly for less breaking change, so user can still do
providerSettings.BalancerType = PersistentStreamProviderConfig.DEFAULT_STREAM_QUEUE_BALANCER_TYPE, when they configure stream provider.
Although this seems like an unusual usage pattern, since without this line it will still be the default value. So I don't have strong option against removing this

jason-bragg · 2017-06-28T00:07:25Z

src/Orleans/Streams/PersistentStreams/IPersistentStreamPullingAgent.cs

@@ -13,7 +13,7 @@ internal interface IPersistentStreamPullingAgent : ISystemTarget, IStreamProduce

    internal interface IPersistentStreamPullingManager : ISystemTarget
    {
-        Task Initialize(Immutable<IQueueAdapter> queueAdapter);
+        Task Initialize(Immutable<IQueueAdapter> queueAdapter, Immutable<IStreamQueueMapper> queueMapper);


We can get the mapper from the adapter, why pass in the additional arg?

U are fast ! wait for 30 mins and come back ! I just submitted this. haven't clean up yet.

sorry for the confusion

As a more straight-forward alternative

jason-bragg · 2017-06-28T00:11:03Z

src/OrleansRuntime/Streams/QueueBalancer/ConsistentRingQueueBalancer.cs

@@ -6,20 +6,26 @@

 namespace Orleans.Streams
 {
-    internal class ConsistentRingQueueBalancer : IAsyncRingRangeListener, IStreamQueueBalancer
+    public class ConsistentRingQueueBalancer : IAsyncRingRangeListener, IStreamQueueBalancer


y make this, and all that it requires, public. This is can/should remain internal, right?

jason-bragg · 2017-06-28T00:15:41Z

src/OrleansRuntime/Streams/QueueBalancer/StreamQueueBalancerFactory.cs

@@ -21,7 +22,7 @@ internal class StreamQueueBalancerFactory
        /// <param name="siloMaturityPeriod">Maturity Period of a silo for queue rebalancing purposes</param>
        /// <returns>Constructed stream queue balancer</returns>
        public static IStreamQueueBalancer Create(


Thought we were getting rid of this class? No longer needed right? Queue manager should be able to get balancer form DI by type, then initialize it directly.

jason-bragg · 2017-06-28T00:16:36Z

src/OrleansRuntime/Streams/QueueBalancer/StreamQueueBalancerType.cs

+    /// <summary>
+    /// Built-in stream queue balancer type which is supported natively in orleans
+    /// </summary>
+    public static class StreamQueueBalancerType


This is where we can define the default queue balancer.

Not sure I follow. Let's talk in person tomorrow

jason-bragg · 2017-06-28T18:49:43Z

src/Orleans/Streams/Providers/IStreamProviderRuntime.cs

-        public StreamQueueBalancerType BalancerType { get; set; } = DEFAULT_STREAM_QUEUE_BALANCER_TYPE;
+
+        /// <summary>
+        /// The queue balancer type for your stream provider. If you are using a custom queue balancer by injecting IStreamQueueBalancer as a transient service into DI,


probably should specify 'transient', we don't really care about the scoping.

I specified transient at the end. scroll 💃

xiazen · 2017-06-28T21:14:18Z

@dotnet-bot test this please

Make StreamQueueBalancer pluggable,

- Make StreamQueueBalancer pluggable,

9487905

- Make it possible for the injected StreamQueueBalancer to be built on top of an async service, such as external storage, or a lease manager.

dnfclas added the cla-already-signed label Jun 21, 2017

xiazen commented Jun 21, 2017

View reviewed changes

change queue balancer name

255a95b

xiazen added this to the 1.5.0 milestone Jun 21, 2017

xiazen requested review from jason-bragg and ReubenBond June 21, 2017 01:53

ReubenBond reviewed Jun 22, 2017

View reviewed changes

PR change

9a0b316

xiazen force-pushed the plugable branch from c679c33 to 9a0b316 Compare June 23, 2017 20:15

xiazen added 2 commits June 23, 2017 13:21

add test with memory stream provider

b69c0b4

clean up

cf4d2b2

xiazen commented Jun 23, 2017

View reviewed changes

jason-bragg reviewed Jun 23, 2017

View reviewed changes

jason-bragg reviewed Jun 26, 2017

View reviewed changes

jason-bragg reviewed Jun 28, 2017

View reviewed changes

Shift configure balancer logic from balancerFactory to its Init method

eaf4387

As a more straight-forward alternative

jason-bragg reviewed Jun 28, 2017

View reviewed changes

xiazen force-pushed the plugable branch from c28024c to eaf4387 Compare June 28, 2017 00:12

jason-bragg reviewed Jun 28, 2017

View reviewed changes

clean up

738c409

xiazen force-pushed the plugable branch from c5dd654 to 738c409 Compare June 28, 2017 00:46

fix tests

0b1ba13

jason-bragg reviewed Jun 28, 2017

View reviewed changes

xiazen force-pushed the plugable branch from 726b047 to 46f74ef Compare June 28, 2017 20:05

remove internal streamqueuebalancerfactory

a74d623

xiazen force-pushed the plugable branch from 46f74ef to a74d623 Compare June 28, 2017 22:35

jason-bragg merged commit d338fb1 into dotnet:master Jun 28, 2017

sergeybykov pushed a commit that referenced this pull request Jul 3, 2017

Make StreamQueueBalancer Pluggable (#3152)

e3e0606

Make StreamQueueBalancer pluggable,

jdom pushed a commit that referenced this pull request Jul 5, 2017

Make StreamQueueBalancer Pluggable (#3152)

28a4062

Make StreamQueueBalancer pluggable,

xiazen deleted the plugable branch November 1, 2017 20:32

github-actions bot locked and limited conversation to collaborators Dec 10, 2023

Make StreamQueueBalancer Pluggable #3152

Make StreamQueueBalancer Pluggable #3152

Conversation

xiazen commented Jun 21, 2017

xiazen Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiazen Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

xiazen Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiazen commented Jun 22, 2017

Choose a reason for hiding this comment

xiazen Jun 22, 2017 • edited Loading

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

xiazen Jun 23, 2017 • edited Loading

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

xiazen Jun 26, 2017 • edited Loading

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

xiazen commented Jun 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jason-bragg Jun 26, 2017 • edited Loading

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

xiazen Jun 26, 2017 • edited Loading

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

xiazen commented Jun 28, 2017

xiazen Jun 21, 2017 •

edited

Loading

xiazen Jun 21, 2017 •

edited

Loading

xiazen Jun 21, 2017 •

edited

Loading

xiazen Jun 22, 2017 •

edited

Loading

xiazen Jun 23, 2017 •

edited

Loading

xiazen Jun 26, 2017 •

edited

Loading

jason-bragg Jun 26, 2017 •

edited

Loading

xiazen Jun 26, 2017 •

edited

Loading