Rust: Support clustered redis #392
Conversation
|
There was a problem hiding this comment.
Haven't reviewed. I'll let @svix-daniel and @svix-dylan take the first jab, but from skimming over it I think it looks good!
There are a lost of unrelated changes though both for spacing and code which make reviewing hard. Can you please fix this?
jaymell
force-pushed
the
clustered-redis3
branch
2 times, most recently
from
5c2f2d2
to
0256c3b
Compare
There was a problem hiding this comment.
I see some notes for things still being cleaned up but this looks good! Between this and #393 we'll have some conflicts at first, but they should be pretty easy to sort out.
jaymell
force-pushed
the
clustered-redis3
branch
4 times, most recently
from
744c70c
to
f92d855
Compare
jaymell
force-pushed
the
clustered-redis3
branch
from
f92d855
to
07b10a9
Compare
Add support for clustered redis connections with `RedisCluster` cache/queue types. Also addd a basic script to run all tests against various queue and cache backend types.
jaymell
force-pushed
the
clustered-redis3
branch
from
07b10a9
to
ae8a907
Compare
| let redis_dsn = || { | ||
| cfg.redis_dsn | ||
| .as_ref() | ||
| .expect("Redis DSN not found") | ||
| .as_str() | ||
| }; |
There was a problem hiding this comment.
This is a bit weird, why have it as a function if it's just borrow in both places?
There was a problem hiding this comment.
Putting the expect in one place instead of in 4 different spots was the goal.
There was a problem hiding this comment.
Why not have:
| let redis_dsn = || { | |
| cfg.redis_dsn | |
| .as_ref() | |
| .expect("Redis DSN not found") | |
| .as_str() | |
| }; | |
| let redis_dsn = | |
| cfg.redis_dsn | |
| .as_ref() | |
| .expect("Redis DSN not found") | |
| .as_str() | |
| ; |
This puts it just one in one place, no?
There was a problem hiding this comment.
This will panic if redis_dsn is not defined.
| @@ -144,11 +165,14 @@ impl TaskQueueSend for RedisQueueProducer { | |||
| let key = to_redis_key(&delivery); | |||
| if let Some(timestamp) = timestamp { | |||
| let _: () = pool | |||
| .zadd(DELAYED, key, timestamp.timestamp()) | |||
There was a problem hiding this comment.
Looking at the worst experience here everywhere by using redis::Cmd::zadd instead of pool.zadd, I wonder if we should have just had our own wrappers for these functions instead of making the experience worst everywhere.
Can probably easily just implement it with a macro.
There was a problem hiding this comment.
What I actually meant is that we can have the behavior shared. So I much rather you to have a shared trait with default implementation that defines a zadd function that internally calls query_async(redis::Cmd::Zadd) so that using the API is less terrible. Does this make sense?
| }; | ||
|
|
||
| tracing::debug!("Cache type: {:?}", cfg.cache_type); | ||
| let cache = match cfg.cache_type { | ||
| CacheType::Redis => { | ||
| let mgr = crate::redis::create_redis_pool(redis_dsn(), false).await; |
There was a problem hiding this comment.
I said it elsewhere but can't see the comment now. Please don't use true/false like this, but rather have a differently named function (internally it can call a private true/false function).
Addresses additional comments from #392.
No description provided.