Add Redis store type #72

omid · 2021-01-18T13:25:52Z

Fixes #68

jaemk · 2021-02-01T02:43:15Z

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.

omid · 2021-02-01T18:49:50Z

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?

omid · 2021-04-21T06:58:08Z

@jaemk ping 🙃

jaemk · 2021-04-27T01:49:39Z

src/stores/redis.rs

+    _phantom: PhantomData<K>,
+}
+
+const ENV_KEY: &str = "REDIS_CS";


can this be optionally specified as a parameter when building the RedisCache struct?

Sure, are you fine that I introduce two new methods, set_prefix and set_connection_string to use instead of these two constants?

jaemk · 2021-04-27T01:53:03Z

src/stores/redis.rs

+                let val = f().await;
+                self.base_set(&key, &val);
+                let index = self.store.len();
+                self.store.push(val);


what's the reason for also storing the value in a vec?

To return a reference to it, since it's in the trait signature. If you know a better way, please don't hesitate.

jaemk · 2021-04-27T01:54:12Z

src/stores/redis.rs

+            }
+            Err(_) => {
+                let val = f().await;
+                self.base_set(&key, &val);


can you make the set and get interaction with redis async?

There is a problem here.

In the redis-rs library, we have two different methods, get_connection which is sync and get_async_connection which is async. The second one will be available if you have the Cargo.toml async feature enabled. There is no problem here :) (ref: 1)

We have some other functions for general Redis commands, like get, which will be sync by default, but will be async based on the trait you use. So same method, but different behaviors. (refs: 1, 2)

Also, I think this Redis implementation should be all async.

But the problem is that there are two sets of cached methods, Cached and CachedAsync traits.
I still don't know why do we have two? Why are we able to disable CachedAsync by a feature flag and not to have it all the time (just compilation speed?)? And cannot we merge them?

If it is possible to merge them and make both sets async, I think all my issues here will be solved.

jaemk · 2021-04-27T01:57:06Z

src/stores/redis.rs

+        self.clean_up();
+        let mut con = self.client.get_connection().unwrap();
+        redis_rs::cmd("EVAL")
+            .arg("local keys = redis.call('keys', ARGV[1]) \n for i=1,#keys,5000 do \n redis.call('del', unpack(keys, i, math.min(i+4999, #keys))) \n end \n return keys")


some comments on what's going on here would be helpful. Is there a benefit of doing this over regular code?

I'll put a comment in the code also.
But it's the way to delete an unlimited number of keys, matching with a pattern, in Redis :|

Copied from here: https://stackoverflow.com/questions/4006324/how-to-atomically-delete-keys-matching-a-pattern-using-redis#comment39607023_16974060

jqnatividad · 2021-12-30T15:22:31Z

Hi @omid , @jaemk !
Just pinging to check if this PR is still WIP...
Thanks!

omid · 2021-12-31T10:15:28Z

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 :(

omid · 2022-01-27T20:31:39Z

I changed this code to support sync Redis for now, until we do support async storages, mentioned here: #98

jqnatividad · 2022-02-12T13:06:09Z

Hi @jaemk , any chance this will make it to the next release?

jaemk · 2022-02-15T02:30:19Z

Still looking this over. I'll try to merge in the next couple of days and will make some additions before releasing.

jaemk · 2022-02-21T20:00:37Z

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.

jaemk · 2022-02-25T04:13:40Z

Okay - released in 0.31.0

Add Redis store type

4a3634e

omid marked this pull request as ready for review January 18, 2021 13:59

Merge remote-tracking branch 'upstream/master' into redis_store

9997c1f

jaemk reviewed Apr 27, 2021

View reviewed changes

omid marked this pull request as draft January 7, 2022 17:47

omid added 5 commits January 16, 2022 19:07

Merge remote-tracking branch 'origin/master' into redis_store

262d9a3

Some tiny tweaks

83bdd1b

Merge

c07b14b

Merge remote-tracking branch 'origin/master' into redis_store

22a0226

sync_redis feature is there!

860b7d4

omid mentioned this pull request Jan 27, 2022

Support Redis #68

Closed

clippy (nightly)

fe5ba9d

omid marked this pull request as ready for review January 27, 2022 20:31

jaemk merged commit ea835b0 into jaemk:master Feb 25, 2022

omid deleted the redis_store branch February 25, 2022 08:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Redis store type #72

Add Redis store type #72

omid commented Jan 18, 2021 •

edited

Loading

jaemk commented Feb 1, 2021

omid commented Feb 1, 2021 •

edited

Loading

omid commented Apr 21, 2021

jaemk Apr 27, 2021

omid Apr 27, 2021 •

edited

Loading

jaemk Apr 27, 2021

omid Apr 27, 2021

jaemk Apr 27, 2021

omid Apr 28, 2021 •

edited

Loading

jaemk Apr 27, 2021

omid Apr 27, 2021

jqnatividad commented Dec 30, 2021

omid commented Dec 31, 2021

omid commented Jan 27, 2022 •

edited

Loading

jqnatividad commented Feb 12, 2022

jaemk commented Feb 15, 2022

jaemk commented Feb 21, 2022 •

edited

Loading

jaemk commented Feb 25, 2022

Add Redis store type #72

Add Redis store type #72

Conversation

omid commented Jan 18, 2021 • edited Loading

jaemk commented Feb 1, 2021

omid commented Feb 1, 2021 • edited Loading

omid commented Apr 21, 2021

jaemk Apr 27, 2021

Choose a reason for hiding this comment

omid Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

jaemk Apr 27, 2021

Choose a reason for hiding this comment

omid Apr 27, 2021

Choose a reason for hiding this comment

jaemk Apr 27, 2021

Choose a reason for hiding this comment

omid Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

jaemk Apr 27, 2021

Choose a reason for hiding this comment

omid Apr 27, 2021

Choose a reason for hiding this comment

jqnatividad commented Dec 30, 2021

omid commented Dec 31, 2021

omid commented Jan 27, 2022 • edited Loading

jqnatividad commented Feb 12, 2022

jaemk commented Feb 15, 2022

jaemk commented Feb 21, 2022 • edited Loading

jaemk commented Feb 25, 2022

omid commented Jan 18, 2021 •

edited

Loading

omid commented Feb 1, 2021 •

edited

Loading

omid Apr 27, 2021 •

edited

Loading

omid Apr 28, 2021 •

edited

Loading

omid commented Jan 27, 2022 •

edited

Loading

jaemk commented Feb 21, 2022 •

edited

Loading