orleans icon indicating copy to clipboard operation
orleans copied to clipboard

Add Redis Stream streaming implementation

Open buzzers opened this issue 2 years ago • 6 comments

This was migrated from the 3.x version I'm using and it should work.

Microsoft Reviewers: Open in CodeFlow

buzzers avatar Apr 20 '23 16:04 buzzers

Many thanks for this PR, @buzzers. We will take a closer look at it tomorrow. Thank you for your patience.

ReubenBond avatar May 18 '23 15:05 ReubenBond

Hi @buzzers ,

It seems that your implementation doesn't guarantee stream event order. While it is true that some providers don't guarantee it (like AzureQueue), it should be the exception (and we will probably separate them from truly rewindable streams in the future).

It appears that the Redis streaming API allow querying from ranges, which would allow us to have a proper rewindable Redis implementation, that would have the same behavior than EventHub.

Do you think you could switch to the range API instead? It would be tremendously valuable to have that in Orleans.

benjaminpetit avatar May 22 '23 16:05 benjaminpetit

@benjaminpetit

Of course, rewindable streams are very valuable. I will refer to EventHub in the near future and try to reimplement it. But here's a question I'm not sure yet: Does the rewindable stream provide an interface to manually clean up messages that are no longer needed? This is important because Redis streams does not provide automatic cleanup of expired messages. And because the messages of Redis streams all exist in memory, if the rewindable stream cannot manually clean up expired messages, there is a high probability that it will cause Redis OOM.

buzzers avatar May 22 '23 17:05 buzzers

For EventHub it is not needed since messages are purged after n days.

But message deletion, if needed, should be done in IQueueAdapterReceiver.MessagesDeliveredAsync() (it seems that's what you did in this PR)

benjaminpetit avatar May 22 '23 18:05 benjaminpetit

@benjaminpetit IQueueAdapterReceiver.MessagesDeliveredAsync() can indeed be used to clean up expired messages. But this might not be very elegant, as Redis commands will be called more often than necessary. But if there is no other better way, there is no problem with cleaning here.

buzzers avatar May 23 '23 09:05 buzzers

Is work on this abandoned? I am really looking forward for this to be merged.... @ReubenBond @benjaminpetit feels like this need some reviewing from you guys

roozbehid avatar Mar 27 '24 16:03 roozbehid