Broadcast channel to replace SMS #7735
Conversation
|
|
||
| namespace Orleans.BroadcastChannel | ||
| { | ||
| public interface IBroadcastChannel<T> |
There was a problem hiding this comment.
I wonder if this should be IBroadcastChannelPublisher or Writer
| _grainFactory = grainFactory; | ||
| _subscriberTable = subscriberTable; | ||
| _fireAndForgetDelivery = fireAndForgetDelivery; | ||
| _logger = loggerFactory.CreateLogger($"{nameof(BroadcastChannel<T>)}-{_channelId}"); |
|
|
||
| public BroadcastChannelConsumerExtension(IGrainContextAccessor grainContextAccessor) | ||
| { | ||
| _subscriptionObserver = grainContextAccessor.GrainContext?.GrainInstance as IOnBroadcastChannelSubscribed; |
There was a problem hiding this comment.
It seems like this is not nullable: it's accessed unconditionally further down.
In that case, perhaps a direct cast would be more appropriate.
| return callback; | ||
| } | ||
| var subscription = new BroadcastChannelSubscription(this, streamId); | ||
| await _subscriptionObserver.OnSubscribed(subscription); |
There was a problem hiding this comment.
The reason this works (BroadcastChannelSubscription.OnSubscribed calls Attach, which adds a handler) is not obvious from looking at the code - a descriptive comment would help
| { | ||
| public static class ChannelHostingExtensions | ||
| { | ||
| public static ISiloBuilder AddBroadcastChannel(this ISiloBuilder @this, string name, Action<BroadcastChannelOptions> configureOptions) |
There was a problem hiding this comment.
Perhaps this should be plural, AddBroadcastChannels, like ASP.NET's AddControllers extension method: https://github.com/dotnet/aspnetcore/blob/3ea008c80d5cc63de7f90ddfd6823b7b006251ff/src/Mvc/Mvc/src/MvcServiceCollectionExtensions.cs#L89
Although there is a difference: this adds support for BroadcastChannel, whereas AddControllers adds the individual controllers (and AddMvc adds support for them)
test/Tester/GrainCallFilterTests.cs
Outdated
| @@ -14,6 +14,7 @@ | |||
| using Xunit; | |||
| using Orleans.Hosting; | |||
| using Orleans.Serialization; | |||
| using Orleans.Providers.Streams.SimpleMessageStream; | |||
There was a problem hiding this comment.
Should we rename this namespace?
|
@benjaminpetit did you want to address the comments? |
ReubenBond
force-pushed
the
simple-stream
branch
from
f324427 to
9873b5a
Compare
benjaminpetit
force-pushed
the
simple-stream
branch
from
9873b5a to
a542c7a
Compare
This PR introduces "Broadcast channels" that will be removed in 4.x (#7475)
The interface for consuming and publishing is more streamlined, but it should behave just like SMS
For now, only implicit subscriptions are supported, but we could add explicit later on.
Microsoft Reviewers: Open in CodeFlow