Feedback: Simplify the Streaming Interfaces
- Dominant language
- C#
- Stars
- 10.9k
- Forks
- 2.1k
- Avg merge
- 14h 42m
- Merged PRs (30d)
- 354
Description
I've been working on #5527 and trying to understand a lot of how the stream provider works, and I wanted to provide some constructive feedback on it. My perspective coming into this is that I'm looking to implement a mostly pure-AMQP stream provider (RabbitMQ) and add it to the project. It's entirely possible that I missed the intent of some of the existing interfaces, they aren't documented well and the formal documentation isn't much more helpful. I wanted to see how far I could get before providing feedback.
Right now with how many design decisions the stream interfaces forces me to make, I've been considering just adding a stream consumer middleware in my system, and then each message received will just call a grain. Personally, I'm on the fence with the streaming provider's readiness for a production workload, but I'm hoping some of the feedback can start to guide the streaming interfaces in the right way.
### Design Decisions
From the outside looking in, the idea of having an internal stream provider which arbitrates on behalf of stream providers is a great idea and I think that was definitely a great decision. However, I think the current implementation forces too many decisions on a given external provider and it forces extension devs to implement things strictly, which isn't that great. I think the big forcing design decisions are `StreamSequenceTokens` and `QueueMapper`.
#### Stream Sequencing
Kafka (to my knowledge) and RabbitMQ don't have support for stream sequencing out of the box, so all messages are FIFO, which should be the normal way of going about things. Stream sequencing is helpful, but not an AMQP trait and definitely a design decision that came from Event Hub. I think stream sequencing should be a provider-internal implementation detail that has no effect on the overall stream message delivery to either the consumer or the cache provider. Especially with RabbitMQ, if I want stream sequencing without a lot of customization, I can't have any type of sharding or partitioning, I get one queue, and that's it. That's not entirely ideal, so requiring extension devs to either explicitly opt out or ignore the feature feels clunky.
#### Cache Provider
I think the cache provider is about halfway done. I would like to see the cache provider have a more CRUD-style interface that has optional overloads for message acknowledgement (key AMQP feature). I think CRUD is the right interface style because it allows a stream provider to perform basic `GET` message to send to a consumer, `UPDATE` (think `basic-ack` from AMQP) where the cache can tell the provider there was a status change on the message (if the provider has support for that), and `DELETE`, meaning a message was read by the consumer.
#### Namespaces and Guids
I think this design decision comes too much from Event Hub and forces design decisions onto stream providers. It seems like there should be one or the other, but not both. Namespaces are hard to map to AMQP, I think they would be topics, and I'm not sure how to map a stream guid (I think it's designed to be a queue?).
I think there should be either namespaces or guids, but I think the right path forward should be guids. I think the stream provider interface should specify a stream guid, and then it's up to the provider to implement it as they see fit. A stream guid means that consumer can get messages from a specific stream guid, and then the provider can map that quid to one or more queues, but the interface shouldn't influence that design.
### Message Wrappers
It definitely feels like there should be a very simple message wrapper, like `IMessageWrapper`, or something like that. It's only job should be to serialized and deserialize the message to and from ways the provider understands how to handle. `IBatchContainer` seems to handle that a bit, but it seems clunky.
### `IQueueAdapter`
```csharp
public interface IQueueAdapter
{
string Name { get; }
Task QueueMessageBatchAsync(Guid streamGuid, String streamNamespace, IEnumerable events, StreamSequenceToken token, Dictionary requestContext);
IQueueAdapterReceiver CreateReceiver(QueueId queueId);
bool IsRewindable { get; }
StreamProviderDirection Direction { get; }
}
```
On this interface, I think the biggest pain point is `QueueMessageBatchAsync`. Not only are the method's parameters undocumented, but it's too heavy. It should provide only the stream guid, a single event payload instead of a enumerable of events, and that's it. The `StreamSequenceToken` forces a provider to handle it (or ignore it, which isn't good either), and the request context likely isn't helpful to the provider. Personally, I'm in favor of dropping both of those arguments and leaving it up to the provider to handle.
`IsRewindable` also feels like a design decision driven by Event Hub. AMQP streams generally aren't designed to be rewindable as they are FIFO, but potentially can be rewindable with a cache. I would argue that should be a function of the cache and not the stream provider.
### `IQueueAdapterFactory`
```csharp
public interface IQueueAdapterFactory
{
Task CreateAdapter();
IQueueAdapterCache GetQueueAdapterCache();
IStreamQueueMapper GetStreamQueueMapper();
Task GetDeliveryFailureHandler(QueueId queueId);
}
```
This one is a bit frustrating as it either requires a stream provider to implement a cache or force them to opt out with `GetQueueCacheAdapter`. A big pain point here is `GetStreamQueueMapper` because it shouldn't matter how a provider handles the mapping, so long as the provider can handle it.
### `IStreamQueueMapper`
```csharp
public interface IStreamQueueMapper
{
IEnumerable GetAllQueues();
QueueId GetQueueForStream(Guid streamGuid, String streamNamespace);
}
```
I appreciate the intent of this interface is to allow a provider to map a stream to a queue, but it feels like too much interference with the stream provider. I think the stream provider should maintain a mapping and be responsible for the implementation details. Maybe I missed why the Orleans runtime needs this, but it definitely feels like it's interfering with how a provider handles the mappings. A provider should receive a message from a stream guid and then it's up to the provider to implement it in their own way.
### Example Desired Interfaces
From my perspective, I want an interface that is very simple and leaves a lot of the details up the stream provider. I'm thinking something like what's below. I think there's a happy medium between what I see as nice to have, and then what exists today.
#### `IStreamMessage`
This interface is really just a wrapper that let's a provider implement a wrapper between how Orleans handles messages and how the provider understands messages. I think anything more than very basic serialization methods on this interface forces too many implementation details onto a provider.
```
public interface IStreamMessage
{
object DeserializeMessage();
object SerializeMessage();
}
```
#### `IStreamPublisher`
This should be a very basic provider interface, which handles the instantiation of the provider and ensuring there are publisher and/or subscriber interfaces attached to it.
```csharp
public interface IStreamProvider
{
IStreamProvider CreateStreamProvider(object options);
IPublisher CreatePublisher(object options);
ISubscriber CreateSubscriber(string streamGuid, string topic);
IPublisher PublishMessageAsync(string streamGuid, IStreamMessage message);
IStreamMessage ReceiveMessage(string streamGuid);
}
```
#### `IPublisher`
This interface should be fairly simple as well, being mostly around publishing to a stream guid with a message associated with it.
```csharp
public interface IPublisher
{
void CreateNewStreamGuid(string streamGuid);
void PublishMessage(IStreamMessage message, string streamNamespace);
Task PublishMessageAsync(IStreamMessage message, string streamNamespace);
}
```
#### `ISubscriber`
This interface would be focused squarely on reading from a given stream guid, either with pull or push methods via callbacks/events.
public interface ISubscriber
{
List GetMessages(int maxCount, string streamNamespace);
Task> GetMessagesAsync(int maxCount, string streamNamespace);
event IStreamMessage Consume(string streamGuid);
}
```
Contributor guide
Assessment
This issue has not been assessed yet.