DefaultRedisCacheWriter.clean() uses blocking KEYS command [DATAREDIS-1151]

[spring-projects-issues]

eyison opened DATAREDIS-1151 and commented

when I use clean method in class DefaultRedisCacheWriter ,it will excute keys command which is block,it may take a lot of time and cause other commands time out

---

Affects: 2.1.17 (Lovelace SR17), 2.2.7 (Moore SR7), 2.3 GA (Neumann)

Reference URL:

spring-data-redis/src/main/java/org/springframework/data/redis/cache/DefaultRedisCacheWriter.java

Line 182 in 2bcb06a

byte[][] keys = Optional.ofNullable(connection.keys(pattern)).orElse(Collections.emptySet())

Issue Links:

• DATAREDIS-1113 Use SCAN instead KEYS when clean cache
  ("is duplicated by")
• DATAREDIS-1176 org.springframework.data.redis.cache.DefaultRedisCacheWriter#clean method may ruin performance
  ("is duplicated by")
• DATAREDIS-1152 change clean method keys command to scan in DefaultRedisCacheWriter
  ("is duplicated by")

Referenced from: pull request #532

1 votes, 4 watchers

[spring-projects-issues]

chengchen commented

Hello, any chance to move this small fix forward? We would very like to have this improvement as well.

We have configured our Redis timeout at 1s, and cleaning the cache is causing timeout quite often.

It would be very appreciated if you could prioritize it, many thanks!

[spring-projects-issues]

Mark Paluch commented

This is indeed a small change looking from a command perspective, but it introduces another level of complexity. KEYS + DEL is atomic while SCAN is not. Keys added while SCAN is in progress leaves entries in the cache. Additionally, we need to partition keys to not run into a timeout when issuing DEL with a huge number of keys. So from that perspective, it's a pretty significant change. Feel free to submit a pull request

[spring-projects-issues]

chengchen commented

Mark Paluch Thanks for the quick feedback!

 KEYS + DEL is atomic

It seems to me that each operation is atomic but between 2 operations you still could leak some entries in the situation you described with SCAN, right? So this issue is only a bit amplified in SCAN IMHO.

 Additionally, we need to partition keys to not run into a timeout when issuing DEL with a huge number of keys

This is the main issue from my perspective. As different users configure different timeouts based on their criteria, it's probably better to leave the batch size configurable. For example, on our side we batch it with 10k keys to DEL, it won't timeout and yields relatively good performance.

 Feel free to submit a pull request.

There is already a PR proposed by someone, but there is some issue with it (DEL 1 by 1)

[spring-projects-issues]

chengchen commented

Forgot to precise: In some cases, it's just not possible to load all the keys in 1 shot (it could simply consume too much memory), at least that's the case for us. That's why we are bypassing the Redis cache and removing entries by ourselves. Obviously, it would be better if it could be addressed by Spring Data Redis

[spring-projects-issues]

chengchen commented

Mark Paluch I think this issue is not as minor as you think :) If the dataset is big enough, you either have timeout and / or OOM errors

[spring-projects-issues]

shenjianeng commented

spring data redis 1.5.x ， maintain known keys is good。

[spring-projects-issues]

chengchen commented

Hello Mark Paluch, hope you are doing well.

I provided an example (not a definite PR) to show what's I have thought about improving this part:

https://github.com/spring-projects/spring-data-redis/pull/547/files

 

Even if we hacked our own way to remove entries for some cache with big quantity, we still rely on Spring Data Redis for some cache with small quantity, but now these cache starts to have this issue as well... So it's kind of important for us to solve this issue from the root. Hope you understand!

[spring-projects-issues]

Mark Paluch commented

After reiterating a few times, it makes sense to reconsider this requirement. The Pull Request at #532 issues a DEL per key, https://github.com/spring-projects/spring-data-redis/pull/547 applies batching. PR 547 also shows the complexity that we need to introduce for batching.

It would make sense to have a way to configure the SCAN batch size and the option to retain the old KEYS behavior. Perhaps the best approach would be introducing a doWithKey method to RedisCacheWriter that accepts a BiFunction<String, RedisConnection> and that is guarded with a lock (if the cache writer is a locking one).

Then, we would extract the current KEY+DEL code and SCAN+DEL code into utility methods and custom implementation can choose from the possible variants

[mp911de]

A potential way out could be introducing a clean strategy that either uses KEYS or SCAN.