-
Notifications
You must be signed in to change notification settings - Fork 134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Redis Streams support #173
Conversation
this looks great to me, I would love to see and extra [ ] for testing performance, I guess we needs some sort of performance suite. |
c00f5dd
to
5adfb17
Compare
eba0551
to
baf8e39
Compare
22ba7d2
to
734fae8
Compare
734fae8
to
daa5d19
Compare
Note that there is some abstraction of streams coming in the redis-rb library. We don't need it for this use case, but could refactor based on it in the future. |
Note that there is a bug in released Redis where listening for updates on streams which have been emptied can cause unpredictable results. At this point, I'm not sure how much it would impact message_bus, but it's worth keeping an eye on: redis/redis@c1e9186 |
The reason that we can't increase the duration for which we block listening for messages on streams is that we rely on regularly polling a separate key which stores our subscription state in order to detect the condition where Redis has been emptied, triggering us to wind back to read the stream from the start again. There are two possible avenues to resolve this:
This is the biggest blocker to proceeding with this PR right now. |
daa5d19
to
1dd89d4
Compare
Implements backlogs using streams rather than sorted sets. Streams do their own length limitation. See https://redis.io/topics/streams-intro.
Streams can be long-polled themselves, without the need to duplicate publishing to a PubSub channel.
Every other test does this.
e30b739
to
18837b2
Compare
@benlangfeld do you feel we should keep this open? I am totally open to adding more backend implementations its just that we have been sitting on this for way too long. |
This change offers an opt-in alternative implementation of MessageBus' Redis backend which uses Redis Streams rather than sorted sets + PubSub.
Redis Streams have constant-time insertion performance, while sorted sets are
O(log(N))
in the size of the set. Streams can also trim themselves, rather than requiring us to trim manually usingZREMRANGEBYSCORE
, which I hope (though have not confirmed) is similarly more performant. Subscribing to messages pushed onto Streams is also more durable than PubSub'sSUBSCRIBE
; global subscribers will not miss messages published during momentary connection failure to Redis, which should make delays in message delivery to subscribed MessageBus clients less of a problem.TODO:
Closes #171.