OrleansContrib / SignalR.Orleans

SignalR backend based on Orleans.
MIT License
295 stars 64 forks source link

Add support for Orleans ISiloBuilder in Generic Host server flow #69

Closed pashchuk closed 5 years ago

pashchuk commented 5 years ago

Currently the extension method .UseSignalR() is only available for ISiloHostBuilder interface. Orleans team has introduced a new builder interface based on the Generic Host package called ISiloBuilder so now we can use new HostBuilder().UseOrleans() extension and co-host the Orleans with any other services in the same application.
It would be great to support this type of config in a SignalR.Orleans package too.

galvesribeiro commented 5 years ago

Yeah, you are partially right :) I've added the generic host support on Orleans :)

So, what you need todo is new HostBuilder().UseOrleans(orleansBuilder => orleansBuilder.UseSignalR()) IIRC.

I'm out on vacation so I can't try now. Could you please check if that work?

Thanks!

stephenlautier commented 5 years ago

@galvesribeiro I'm also working on converting our projects to ours, and run into the same issue.

No, unfortunately, that doesn't work as in Orleans they created a new version which is ISiloBuilder and not ISiloHostBuilder, so the extension methods doesnt work.

I was going to implement the generic host extensions

galvesribeiro commented 5 years ago

Yeah, I was the one which added the Generic Host support on Orleans 2.3.0. I'm curious to see which problems are happening as we made sure it wont break the extensions that much.

Anyway, if there is a change to be made here and you want to add it, feel free. Thanks!

stephenlautier commented 5 years ago

It would be less of an issue if there was some kind of an interface across between ISiloHostBuilder and ISiloBuilder and all shared extensions method are changed to that (such as UseSignalR) so they are available on both.

Like this, all extension methods are authored on ISiloHostBuilder and every library needs to support both

galvesribeiro commented 5 years ago

Wait... I'm confused... Don't we already support the ISiloHostBuilder?

https://github.com/OrleansContrib/SignalR.Orleans/blob/master/src/SignalR.Orleans/Extensions.cs#L13

What am I missing?

It looks the same on the other extensions I maintain here:

https://github.com/OrleansContrib/Orleans.CosmosDB/blob/master/src/Orleans.Clustering.CosmosDB/ClusteringExtensions.cs#L13

stephenlautier commented 5 years ago

ISiloHostBuilder is the "old" .. ISiloBuilder is the new (note without Host)

image

galvesribeiro commented 5 years ago

Yeah, my bad. Now I see the difference. I wonder why nobody complained about that on the other extensions which is even more widely used than this one.

stephenlautier commented 5 years ago

My guess is most probably most of the people are still not using generic host, as there weren't even the AddIncomingGrainCallFilter

So far i had issues already with 3 plugins on a small app just for a POC, not having extension methods on the generic host correctly

stephenlautier commented 5 years ago

Just of curiosity, any idea since you're quite involved with orleans the way i had suggested was it brought up?

e.g. having an interface shared across so extensions are shared?

galvesribeiro commented 5 years ago

Now I remember a discussion we had when we were planning this integration... We opted to not change the current interfaces/extension methods in a breaking way. Instead, we decided to just add new extension methods for the new interface.

Look here that we have two extension methods on each case. One for ISiloBuilder and other for ISiloHostBuilder.

That way, we don't add extra breakage... Perhaps we should have the same here...