Migrate to github.com/rueian/rueidis #262
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #262 +/- ##
==========================================
+ Coverage 83.55% 84.29% +0.73%
==========================================
Files 36 37 +1
Lines 8234 7876 -358
==========================================
- Hits 6880 6639 -241
+ Misses 1025 914 -111
+ Partials 329 323 -6
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Single instance Redis benchmarks (redigo vs rueidis): |
|
The same (single Redis) with some optimizations from latest commit: |
…e into migrate_to_rueidis
|
Redis Cluster benchmarks (redigo vs rueidis). (some missing as bench run timed out) |
Adressed this in 98bda70, now cmp is: |
|
Hi, This is great, I don't work on the same project anymore, I will let my colleague follow this up. Thank you :) |
|
Addressed Redis Survey benchmarks in d03b278, made implementation match to what we have in redigo case - NumCPU workers to process control messages. |
First comparison between
|
|
Was able to tune |
|
Hi, this is a really exciting improvement! rueidis is impressively better than go-redis in almost all cases, I agree it is a better solution. But there is still one case where rueidis is slower, maybe there is some room for optimization:
|
|
@j178 thanks, I'll try to come with detailed comment with examples about this sharding very soon then |
|
So here are options we have in type RedisBrokerConfig struct {
...
// NumPubSubShards defines how many PUB/SUB shards will be used by Centrifuge.
// Each PUB/SUB shard uses dedicated connection to Redis. Zero value means 1.
NumPubSubShards int
// NumPubSubSubscribers defines how many subscriber goroutines will be used by
// Centrifuge for each PUB/SUB shard. Zero value tells Centrifuge to use 16
// subscriber goroutines per PUB/SUB shard.
NumPubSubSubscribers int
// NumPubSubProcessors allows configuring number of workers which will process messages
// coming from Redis PUB/SUB. Zero value tells Centrifuge to use the number calculated as
// runtime.NumCPU / NumPubSubShards / NumClusterShards (if used) (minimum 1).
NumPubSubProcessors int
// NumClusterShards when greater than zero allows turning on a mode when broker
// will use Redis Cluster with sharded PUB/SUB feature. To achieve PUB/SUB
// efficiency RedisBroker reduces possible Redis Cluster slots to the NumClusterShards
// value and starts a separate PUB/SUB for each shard. By default, sharded PUB/SUB is
// not used - i.e. we use globally distributed PUBLISH commands in Redis Cluster.
// Note, that turning on NumClusterShards will cause Centrifuge to generate different
// keys and Redis channels than in the base Redis Cluster mode since we need to control
// slots and be sure our subscriber routines work with the same Redis Cluster slot - thus
// making it possible to subscribe over a single connection.
// This feature requires Redis >= 7.
NumClusterShards int
}Also type RedisBroker struct {
...
config RedisBrokerConfig
shards []*shardWrapper
}RedisBroker.shardsLet's start from This is sth we had here before. RedisBrokerConfig.NumPubSubSubscribers
RedisBrokerConfig.NumPubSubProcessors
RedisBrokerConfig.NumClusterShardsThis is most tricky I suppose. Redis Cluster has sharded PUB/SUB. So where previously we had to use several Though Redis sharded PUB/SUB feature does not come for free in our case. We can not issue isolated SSUBSCRIBE commands and listen for changes from Redis. Because we may have millions of channels on one Centrifuge node. And we still need to synchronize issuing TBH I don't like this approach a lot in its current state as it involves some thinking from developers how many cluster slots they need. There may be a theoretical situation when the number of Redis nodes in Redis cluster grows above initially selected RedisBrokerConfig.NumPubSubShardsTurned out that PUB/SUB throughput may be maximized if we additionally split message passing over several connections (by default we use a single connection for subscribing/receiving in our subscriber goroutines). To be honest it's hard for me to explain where exactly the benefit comes from - but @j178 hope the motivation is a bit more clear now. Please feel free to ask so I could expand sth |
Hi @FZambia, I just have a look into the For example, storing the func (b *RedisBroker) runPubSub(s *shardWrapper, eventHandler BrokerEventHandler, clusterShardIndex, psShardIndex int, useShardedPubSub bool) {
...
conn, cancel := s.shard.client.Dedicate()
defer cancel()
defer conn.Close()
wait := conn.SetPubSubHooks(rueidis.PubSubHooks{...})
s.subChannels[clusterShardIndex][psShardIndex].conn.Store(conn)
close(s.subChannels[clusterShardIndex][psShardIndex].stored)
...
}and use it at func (b *RedisBroker) sendSubscribe(s *shardWrapper, r subRequest, clusterShardIndex, psShardIndex, subscriberShardIndex int) error {
<-s.subChannels[clusterShardIndex][psShardIndex].stored
conn := s.subChannels[clusterShardIndex][psShardIndex].conn.Load().(rueidis.DedicatedClient)
return conn.Do(context.Background(), conn.B().Subscribe().Channel(*(*[]string)(unsafe.Pointer(&r.channels))...).Build()).Error()
}Here is benchmark result comparing to master branch on my macbook: |
|
@rueian many thanks, I'll try to evaluate whether it's possible to do this way. Can't understand quickly as there is some logic for resubscribing to channels upon connection lost – and it's hard to quickly realize is it safe to avoid going through runPubSub pipeline. BTW, in the blog post benchmarks I ran Redigo benchmarks against https://github.com/centrifugal/centrifuge/tree/redigo_optimized branch where I tried to optimize Redigo-based engine we have in master before comparing it to Rueidis. Could you re-run the bench compared to that branch? I believe Rueidis still should perform better as we bypass some code, but Redigo should be closer. |
Sure. I also notice that there are some configuration differences between the two branches:
The following result uses |
|
@rueian I tried approach with saving conn to |
That was quit surprising.
Sure, please let me know if you need other help. I am also planing to release v0.0.86 this weekend. |
FZambia
changed the title
|
In 9db9225 I made some parts of the work done here non-public for now. This includes:
|
This was referenced Nov 28, 2022
Closed
@rueian it's a never-ending work... I just discovered the reason why Redigo was better in Subscribe – it does not wait for Subscribe response from Redis synchronously. Only flushes command to the network. It's pretty surprising for me, and actually can lead to the bug in Centrifugo since I relied on the fact whether subscribe op returned error or not - so theoretically client connection could receive subscribe success while actually result is unknown. Since Rueidis waits synchronously it results in worse subscribe throughput under increased latency between app and Redis. I decided to give the approach you suggested above one more shot, using Rueidis Client directly without intermediate pipelines - it provides better throughput. |
Oh, that is not very surprising to me though. Actually, rueidis also didn't wait subscribe results until we fixed the redis/rueidis#55. The pubsub protocol makes the desired behavior hard to be implemented.
I believe it is not a bad thing... At least we know more about these libraries. I am also working on improving rueidis' flush delay mechanism we added last week by looking into historical statistics: redis/rueidis#159 It now performs better in your benchmark with low parallelism, especially when setting |
But it will work the same way as now in the future right? |
|
Sure, rueidis will keep the current behavior - waiting for subscribe results. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
An alternative approach to #235 + Sharded PUB/SUB support (relates #228). Relates #210.
@j178 hello, I was already working on replacing
redigotorueidisfor some time before you opened #235, butrueidisdid not support RESP2 till the v0.0.80 release (published today). So it seems a viable alternative togo-redisnow.This PR contains an approach to scale PUB/SUB in Redis Cluster using Rediis 7 sharded PUB/SUB feature. Probably it should be a separate pr - but just had it already... In sharded PUB/SUB case we split the key space of broker to configured
NumClusterShardsparts. Then we start separate PUB/SUB routines to work with each PUB/SUB shard. This means that instead of 16384 slots (usually used by Redis Cluster) we haveNumClusterShardsslots for all broker ops. This still limits scalability a bit (though I think sth like NumClusterShards == 16/32/64 is OK for many use cases) but otherwise we would require too many connections to deal with PUB/SUB for millions of channels. It's also worth noting that turning on NumClusterShards option to a value greater than zero will result into different keys to keep values in Redis than keys we have when not using sharded PUB/SUB.I am personally biased towards
rueidisat the moment, though still have not put enough effort into testing and comparing both implementations. I'll try to update this pr with comments as I proceed with some checks.I also removed
IdleTimeoutandUseTLSoptions from Redis shard config as they do not matchrueidisoptions (matched to redigo options currently). And added possibility to set ClientName@tony-pang, hello - are you still interested in sharded PUB/SUB implementation?