Closed omid closed 2 years ago
Thanks for taking this on @omid ! Sorry, I haven't had much time recently to review this. This looks like a great start. There's a couple things I'd to iron out:
redis configuration:
const ENV_KEY: &str = "REDIS_CS";
const PREFIX: &str = "cached_key_prefix-";
I'd like if we could push this logic into the Redis store type -- basically, make it as flexible as possible for the calling code to specify configuration parameters (connection string, cache-prefix, any other options we may want to add in the future) by either (or some combination of): 1. Pass an env var to use, 2. Pass a raw connection string, 3. Pass a callable that returns a config object
Support tokio/async-std by passing though a feature flag to redis-rs
Using redis-rs async methods when used by async macros
Another thing to think about (probably out of scope for this PR, so just something to keep in mind for now) is connection pooling. I don't think redis-rs supports it out of the box, instead requiring you to use an r2d2 or mobc wrapper. It's a tricky problem, but what I'd like to eventually allow is for calling code to provide a pool of some sort so this library isn't doing any resource management, instead letting the caller handle it. This might mean having to understand a raw connection vs r2d2-pool vs mobc-pool, or providing a "RedisCache" trait to abstract over the three, this lib could provide impl's for the common ones but always allow callers to provide their own.
Thanks @jaemk :)
redis configuration
Sure, I'll introduce two methods, set_prefix
and set_connection_string
to set these two values. Maybe later there will be more wishes :D
Support tokio/async-std by passing though a feature flag to redis-rs
You mean to have two different features, like redis-tokio
and redis-async
in cached
and pass it to the redis-rs
?
Using redis-rs async methods when used by async macros
Sure 👍🏼
connection pooling
You are right, we need this and redis-rs doesn't have it yet. And as you said, let's do it in another PR. 🤔
/// I think it's better to switch to https://docs.rs/redis/0.19.0/redis/aio/struct.ConnectionManager.html . It's only async, do you think it's a good idea to also use it in the not-async methods?
@jaemk ping 🙃
Hi @omid , @jaemk ! Just pinging to check if this PR is still WIP... Thanks!
Hey @jqnatividad The code in the PR doesn't work and is not ready. Maybe I can convert it to draft. And sadly I didn't have time to finish it :(
I changed this code to support sync Redis for now, until we do support async storages, mentioned here: https://github.com/jaemk/cached/issues/98
Hi @jaemk , any chance this will make it to the next release?
Still looking this over. I'll try to merge in the next couple of days and will make some additions before releasing.
I haven't forgotten about this, it's just taking me a bit longer than I expected - incorporating some ideas mentioned in #98 so the implementation doesn't need to go through as many hoops to fit the Cached trait. My plan is still the same though, will merge this PR shortly and then apply some patches before releasing.
Okay - released in 0.31.0
Fixes #68