Persistent stream provider to lifecycle by jason-bragg · Pull Request #4042 · dotnet/orleans

jason-bragg · 2018-02-15T02:52:04Z

Pushing this PR out to get eyes on it, but it's not passing testing yet. Still WIP.

Since persistent stream provider is the base for most of our steam providers, we couldn't refactor these one at a time.

Following stream providers now use options and lifecycle.

Azure Queue
EventHub
Sqs
PubSub
Memory
Generator
EventDataGenerator
Various test providers

TBD:
Fix tests.
Move options to Orleans.Configuration namespace.
Replace DataConnectionStream with ConnectionString in options to conform to other options.
Possibly validators, but would like to push that to separate PR, given the size of this change.

NOTE: Persistent stream providers and various adapters are complex and patterns can be improved now that we can inject dependencies, but current patterns are best effort port. Please note where further refactoring in the future is warranted.

Future Refactor:

Data Adapters
Inject checkpointing
Inject generators (for generated streams)
Inject failure handlers
Possible adapter or provider builders

ReubenBond · 2018-02-15T02:54:36Z

The last big PR before we tag an RC 🎉

You can put [WIP] in the title to mark it as WIP if you want

jason-bragg · 2018-02-15T02:56:16Z

Side note: There is lots of crazy in all this, but the updated pattern is much cleaner and gives us a good foundation for future improvements.

@galvesribeiro, can you spare a glance at the external stream providers you've added. Maybe plan in some time to help me test them later in the week? It would be much appreciated! :)

ReubenBond · 2018-02-15T02:59:08Z

src/AWS/Orleans.Streaming.SQS/Hosting/ClientBuilderExtensions.cs

+        /// <summary>
+        /// Configure cluster client to use SQS persistent streams.
+        /// </summary>
+        public static IServiceCollection AddClusterClientSqsStreams(this IServiceCollection services, string name,


I think we should not add any extension methods which hang from IServiceCollection and we should endeavor to remove the existing ones.

(I'd be happy if they were private or internal)

Sorry, not following... Why?

Because otherwise we will have methods which are ambiguous between client & silo & SignalR & ASP.NET. I'm thinking about the future, but the issue is present here today: we need to name the methods differently for client and silo instead of having a nice simple "AddSqsStreams" method.

I see... Well, other framework (including asp.net) does that IIRC. However, I think we should have diff assemblies for client and silo for the providers. For example, in Membership, makes no sense to bring the Membership table to the client, but instead it should be only have the gateway. So Orleans.Clustering.XXXX and Orleans.Clustering.XXXX.Client(or Gateway). I know, extra nuget package to maintain, but the benefits are:

Avoid this confusion with extensions

Avoid having 2 methods for client and silo

In the case of clustering, remove the dependency on the runtime/silo assemblies on teh client.

Will discuss then address in separate PR based on consensus.

Maybe the extension methods should at least live in their own namespace? One for the clientbuilder and one for the silo? Better for discoverability.

galvesribeiro · 2018-02-15T03:02:07Z

@jason-bragg absolutely. Will grab your branch here and check it. Meanwhile, feel free to ping me on skype of gitter to talk about it.

ReubenBond · 2018-02-15T03:03:20Z

src/AWS/Orleans.Streaming.SQS/Streams/SqsStreamOptions.cs

+
+namespace Orleans.Hosting
+{
+    public class SqsStreamOptions : PersistentStreamOptions


Move options to the Orleans.Configuration namespace

ReubenBond · 2018-02-15T03:04:45Z

src/AWS/Orleans.Streaming.SQS/Streams/SqsStreamOptions.cs

+                formatted.AddRange(new[]
+                {
+                    OptionFormattingUtilities.Format(nameof(this.options.DeploymentId), this.options.DeploymentId),
+                    OptionFormattingUtilities.Format(nameof(this.options.DeploymentId), "<-Redacted->"),


Error here, duplicated line. I think the latter is supposed to be DataConnectionString. We can use the existing ConfigUtils.RedactConnectionStringInfo method as in other providers - unless you feel it isn't adequate?

Good catch on the dupe, tnx.
The redact utility, by my understanding, expects an azure connection string, and knows how to pull the secrets out of that. I don't know if we can trust it to redact secrets from other service connection strings. It would be valuable to be able to strip out secrets for all of these, but I don't have the time to investigate each format, so I've been just placing the generic <-redact-> line in there.

I added logic in ConfigUtils.RedactConnectionStringInfo to just return <--SNIP--> by default if none of those format we are looking for is there. And i'm off the opinion we should only keep the default logic.

That's quite fair - I simply don't know how to ensure that we can redact SQS keys correctly, or if we even should. Redacting the entire thing is safer

Looking at the RedactConnectionStringInfo logic, I'm not confident it's safe. If a connection string has one secret in the list, and one not in the list, it's secrets will be logged. I'm going to stick with the <--SNIP--> for now. Developers/maintainers of a component can develop component specific redactors if it's important. IMO it's better to log less info than risk logging users secrets.

ReubenBond · 2018-02-15T03:11:22Z

src/AWS/Orleans.Streaming.SQS/Streams/SqsStreamOptions.cs

+    {
+        public string DeploymentId { get; set; }
+
+        public string DataConnectionString { get; set; }


Since this is provider-specific, could we call this ConnectionString instead?

I think it should not be standardazied as connection string on all providers... others don't have it but instead, they ask for something like endpoint and API key... for those cases, our options should reflect that with 2 properties just like I did with the CosmosDB providers.

This DataConnectionString there was a hack back in the day just to "follow the standard".

I'll submit a PR to @jason-bragg branch changing that as well

Yes, specific names per-provider is fine. Whatever is the typical lingo for that system is what we should use.

ReubenBond · 2018-02-15T03:14:19Z

src/AWS/Orleans.Streaming.SQS/Streams/SqsStreamOptions.cs

+{
+    public class SqsStreamOptions : PersistentStreamOptions
+    {
+        public string DeploymentId { get; set; }


Should this value be configured here or should it come from some other, more central place? My understanding is that this should be replaced with SiloOptions.ClusterId, since we smooshed DeploymentId and ClusterId together here: #3728

Yes, this value should be dropped from there and inject the SiloOptions. The newer providers are already doing that. Will do that on a PR.

jason-bragg · 2018-02-15T07:31:36Z

@galvesribeiro, I've done very little testing with this so far. I should have it stable enough for you to try by mid-day tomorrow. If you want to play with it before then, you're welcome to do so, but I'd expect there to be issues outside the providers I requested your help on. Just don't want to use up your patience on issues I should be solving. :)

galvesribeiro · 2018-02-15T11:32:40Z

@jason-bragg Ok. Take your time. Once you feel it is ready let me know and I'll amend a PR to your branch. I will run all tests as I've essentially ported all providers to .net core back in day and not just the ones I introduced and then will report back.

veikkoeeva · 2018-02-15T17:16:46Z

I'll page @joeybrown, since he is working on #634.

xiazen · 2018-02-15T18:32:19Z

woke up today and a BIG present is awaiting on GitHub
and .. how to insert emoji here ?! lol
good work @jason-bragg !

jason-bragg · 2018-02-15T18:34:33Z

That was my valentines day present to the team/community.. I'm so romantic, as you can all see.

xiazen · 2018-02-15T19:33:15Z

src/AWS/Orleans.Streaming.SQS/Hosting/SiloBuilderExtensions.cs

+            Action<OptionsBuilder<SqsStreamOptions>> configureOptions = null)
+        {
+            return services.TryConfigureFormatterResolver<SqsStreamOptions, SqsStreamOptionsFormatterResolver>()
+                           .AddSiloPersistentStreams<SqsStreamOptions>(name, SQSAdapterFactory.Create, configureOptions);


missing ConfigureNamedOptionForLogging

jason-bragg · 2018-02-15T21:10:08Z

Heads up, for anyone tinkering with this, PR is hilariously broken, as provider initialization needs be run in Orleans scheduling context, which is still under development. I'll update the thread when there is a faint chance this works :/

xiazen · 2018-02-15T21:13:25Z

src/AWS/Orleans.Streaming.SQS/Streams/SQSAdapterFactory.cs

+        {
+            IOptionsSnapshot<SqsStreamOptions> streamOptionsSnapshot = services.GetRequiredService<IOptionsSnapshot<SqsStreamOptions>>();
+            var factory = ActivatorUtilities.CreateInstance<SQSAdapterFactory>(services, name, streamOptionsSnapshot.Get(name));
+            factory.Init();


should factory handle its init or lifecycle?

Init is not async, so no reason I can see to link adapters to lifecycle, they are just pluggable components.

xiazen · 2018-02-15T21:15:17Z

src/AWS/Orleans.Streaming.SQS/Streams/SqsStreamOptions.cs

+            return new Formatter(name, optionsSnapshot.Get(name));
+        }
+
+        public class Formatter : IOptionFormatter<SqsStreamOptions>


formatter name should has its option name

Subclass, so scoped with proper name.

xiazen · 2018-02-15T21:17:24Z

src/AWS/Orleans.Streaming.SQS/Streams/SqsStreamOptions.cs

+                formatted.AddRange(new[]
+                {
+                    OptionFormattingUtilities.Format(nameof(this.options.DeploymentId), this.options.DeploymentId),
+                    OptionFormattingUtilities.Format(nameof(this.options.DeploymentId), "<-Redacted->"),


I added logic in ConfigUtils.RedactConnectionStringInfo to just return <--SNIP--> by default if none of those format we are looking for is there. And i'm off the opinion we should only keep the default logic.

xiazen · 2018-02-15T21:18:07Z

src/Azure/Orleans.Streaming.AzureStorage/Hosting/ClientBuilderExtensions.cs

+            Action<OptionsBuilder<AzureQueueStreamOptions>> configureOptions = null)
+            where TDataAdapter : IAzureQueueDataAdapter
+        {
+            return services.TryConfigureFormatterResolver<AzureQueueStreamOptions, AzureQueueStreamOptionsFormatterResolver>()


missing ConfigureNamedOptionForLogging

xiazen · 2018-02-15T21:18:33Z

src/Azure/Orleans.Streaming.AzureStorage/Hosting/SiloBuilderExtensions.cs

+            Action<OptionsBuilder<AzureQueueStreamOptions>> configureOptions = null)
+           where TDataAdapter : IAzureQueueDataAdapter
+        {
+            return services.TryConfigureFormatterResolver<AzureQueueStreamOptions, AzureQueueStreamOptionsFormatterResolver>()


many occurrence of missing ConfigureNamedOptionForLogging

it was being done in AddPersistentStreams, but that's not the right place. They should be there now.

xiazen · 2018-02-15T21:53:33Z

src/Orleans.Core/Streams/PersistentStreams/PersistentStreamProvider.cs

+            this.logger = logger;
+        }
+
+        private async Task Init(CancellationToken token) 


what's your plan on help user migrate their persistent stream provider, which they developed on top of the old streaming interfaces, to this new one?

Most of the adapter interfaces have not changed. The main difference is that they'd need to move to constructor injection as nothing is passed in during the init, and add an options class for their adapter. These are breaking changes, but not dramatic ones (imo).

xiazen · 2018-02-15T21:55:42Z

src/Orleans.Core/Streams/PersistentStreams/PersistentStreamProviderOptions.cs

+        public RunState StartupState = DEFAULT_STARTUP_STATE;
+        public const RunState DEFAULT_STARTUP_STATE = RunState.AgentsStarted;
+
+        public int InitStage { get; set; }


we should set default stages. This is advanced feature only advanced users should touch. We shouldn't create extra friction for less advanced users to understand lifecycle and configure a correct stage

xiazen · 2018-02-15T22:00:57Z

src/Orleans.Runtime/Streams/PersistentStream/PersistentStreamPullingManager.cs

        private IStreamQueueBalancer queueBalancer;
        private readonly IQueueAdapterFactory adapterFactory;
-        private PersistentStreamProviderState managerState;
+        private RunState managerState;


RunState? should just state be sufficient here ? What does run imply here ?

it replaces PersistentStreamProviderState and tracks if the provider is running or not.

xiazen · 2018-02-15T22:06:12Z

src/Orleans.Runtime/Streams/QueueBalancer/LeaseBasedQueueBalancer.cs

-            var balancerConfig = new LeaseBasedQueueBalancerConfig(providerConfig);
-            this.leaseProvider = this.serviceProvider.GetRequiredService(balancerConfig.LeaseProviderType) as ILeaseProvider;
-            this.leaseLength = balancerConfig.LeaseLength;
+            var options = this.serviceProvider.GetServiceByName<LeaseBasedQueueBalancerOptions>(strProviderName)


we should have providerBuilder and provider lifecycle. :)

There's too many components in streams. The complexity can benefits from lifecycle and builder pattern, in both perspective of configuring and start and shutdown sequence.

Some of those are managed by Silo(Client)ProviderRuntime, some are managed by the PersistentStreamProvider. Those unnecessary coupling partially result in bad consequence, such as streaming infra has to live in core.

Just throwing ideas out there, not necessary the scope of this PR.

I'm not convinced of the lifecycle aspect, but it's definitely worth consideration. In the description I've started a list of further refactoring we may want to do in the future. Please feel free to edit the description to add more suggestions.

xiazen · 2018-02-15T22:10:44Z

src/OrleansProviders/Streams/Generator/GeneratorAdapterFactory.cs

            if (this.ReceiverMonitorFactory == null)
                this.ReceiverMonitorFactory = (dimensions, telemetryProducer) => new DefaultQueueAdapterReceiverMonitor(dimensions, telemetryProducer);
-            if (adapterConfig.GeneratorConfigType != null)
+            generatorConfig = this.serviceProvider.GetServiceByName<IStreamGeneratorConfig>(this.Name);


constructor injection instead of this.

generator config is not required, it can be provided at runtime, hence constructor injection will not work :/

jason-bragg · 2018-02-16T01:28:19Z

@galvesribeiro I think core is solid enough for you to test against in the latest PR. Still has test failures, and hacks i'm resolving, but it's mostly their I think..

galvesribeiro · 2018-02-16T01:46:50Z

@jason-bragg sure, will do my changes in a PR early tomorrow. Probably when you come to office it should be there. Thanks!

xiazen · 2018-02-16T02:45:23Z

@jason-bragg Submitted a PR to your branch regarding migrate SMS stream provider to use option and lifecycle.
Nothing urgent there, just wanted to get that out before I left tomorrow. That pr will likely be resubmitted as a pr against master after your branch has been merged .
Feel free to take a look . Code is complete. I haven't run tests yet.

galvesribeiro · 2018-02-18T15:35:03Z

Sorry for my delay. The PR with the AWS updates were made to your branch @jason-bragg (jason-bragg#4).

I've also ran other provider tests and they looks fine so far.

jason-bragg · 2018-02-20T22:04:43Z

Ready (assuming tests pass)

galvesribeiro · 2018-02-20T22:07:04Z

@jason-bragg did you saw the PR on your branch?

jason-bragg · 2018-02-20T22:22:51Z

@galvesribeiro, I think. I was a bit confused.
It looks like a general port of providers, which is mostly orthogonal to this PR. Am I mistaken?
There are two remaining, somewhat related sets of work not included in this PR; the provider ports you did, and the SMS port performed by Xiao. I'm working to stabilize this change and get it in, then I'll work with you and Xiao to get the other sets of work in.

xiazen · 2018-02-21T00:00:03Z

@dotnet-bot test functional please

galvesribeiro

LGTM

ReubenBond · 2018-02-21T01:27:22Z

src/AWS/Orleans.Streaming.SQS/Streams/SqsStreamOptions.cs

Should this be [RedactConnectionString]? Xiao made a change to redact the entire thing by default if no redactions were made.

BTW, @xiazen, that change also means that UseDevelopmentStorage=true would be redacted, but better to be safe than sorry.

I don't think so. I'm unconvinced RedactConnectionString is safe for unknown formats. By the logic of the call, if any of the patterns match the string is logged with the recognized patterns redacted. This means if a connection string has a matched, and an unmatched secret, the unmatched secret will logged. which is not acceptable.

ok, no worries. In the future we can create a version which only includes known-good portions of the connection string, instead. We can wait until someone complains, if they ever do.

jason-bragg · 2018-02-21T03:35:08Z

Functionals seem to be failing again due to test timeout. Pushing a possible optimization of lifecycle to see if it helps. If it doesn't I'll role it back.

xiazen · 2018-02-21T18:41:34Z

I kicked out a test build in vso to run azure tests, which were skipped here. Also hoping the time out in vso is longer, so the build won't fail due to test timeout

jason-bragg · 2018-02-21T19:10:44Z

Functionals timed out again. Trying something different. This time I'm running silo lifecycle actions outside the lifecycle scheduling context.

jason-bragg · 2018-02-21T19:51:13Z

hmm.. functionals seem say they took 30~ish minutes.. I don't trust this.

- Was not setting the blancer type.

jason-bragg · 2018-02-21T23:48:08Z

Removed lifecycle perf fix, and rebased. Should be good to merge after tests run.

jason-bragg added the Status: work-in-progress label Feb 15, 2018

jason-bragg assigned xiazen Feb 15, 2018

jason-bragg requested review from ReubenBond, benjaminpetit, sergeybykov and xiazen February 15, 2018 02:52

jason-bragg changed the title ~~Persistent stream provider to lifecycle~~ WIP - Persistent stream provider to lifecycle Feb 15, 2018

ReubenBond reviewed Feb 15, 2018

View reviewed changes

xiazen reviewed Feb 15, 2018

View reviewed changes

jason-bragg force-pushed the PersistentStreamProviderToLifecycle branch from 062cf40 to ed644c4 Compare February 16, 2018 02:52

jason-bragg force-pushed the PersistentStreamProviderToLifecycle branch from d5eede5 to b337ea1 Compare February 20, 2018 19:21

jason-bragg removed the Status: work-in-progress label Feb 20, 2018

galvesribeiro mentioned this pull request Feb 20, 2018

DynamoDB transaction log #4056

Merged

jason-bragg changed the title ~~WIP - Persistent stream provider to lifecycle~~ Persistent stream provider to lifecycle Feb 20, 2018

xiazen approved these changes Feb 20, 2018

View reviewed changes

galvesribeiro approved these changes Feb 21, 2018

View reviewed changes

sergeybykov added this to the 2.0.0-rc milestone Feb 21, 2018

ReubenBond reviewed Feb 21, 2018

View reviewed changes

ReubenBond approved these changes Feb 21, 2018

View reviewed changes

jason-bragg force-pushed the PersistentStreamProviderToLifecycle branch from c191b02 to ecff572 Compare February 21, 2018 19:08

jason-bragg mentioned this pull request Feb 21, 2018

Sms stream provider to lifecycle #4064

Merged

jason-bragg added 2 commits February 21, 2018 15:46

Replaced AzureTableStorage with AzureTableGrainStorage.

83fff96

Fixed plugable queue balancer test failures.

82c4335

- Was not setting the blancer type.

jason-bragg force-pushed the PersistentStreamProviderToLifecycle branch from ecff572 to 82c4335 Compare February 21, 2018 23:46

sergeybykov merged commit 4cbfb2c into dotnet:master Feb 22, 2018

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

Conversation

jason-bragg commented Feb 15, 2018

Uh oh!

ReubenBond commented Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jason-bragg commented Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ReubenBond Feb 15, 2018 • 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!

galvesribeiro commented Feb 15, 2018

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!

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 Feb 15, 2018

Uh oh!

galvesribeiro commented Feb 15, 2018

Uh oh!

veikkoeeva commented Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiazen commented Feb 15, 2018

Uh oh!

jason-bragg commented Feb 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jason-bragg commented Feb 15, 2018

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ReubenBond commented Feb 15, 2018 •

edited

Loading

jason-bragg commented Feb 15, 2018 •

edited

Loading

ReubenBond Feb 15, 2018 •

edited

Loading

veikkoeeva commented Feb 15, 2018 •

edited

Loading

xiazen commented Feb 16, 2018 •

edited

Loading