Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redis cache implementation should not use KEYS command #45783

Closed
holomekc opened this issue Jan 22, 2025 · 4 comments · Fixed by #45828
Closed

Redis cache implementation should not use KEYS command #45783

holomekc opened this issue Jan 22, 2025 · 4 comments · Fixed by #45828
Assignees
Labels
Milestone

Comments

@holomekc
Copy link
Contributor

holomekc commented Jan 22, 2025

Description

At the moment the Redis Cache implementation uses the KEYS command.
That is maybe not the best approach. See for example: https://docs.keydb.dev/blog/2020/08/10/blog-post/#:~:text=KEYS%20vs%20SCAN&text=The%20KEYS%20command%20and%20SCAN,the%20length%20of%20the%20query.

Implementation ideas

Instead the SCAN command should be used and maybe also it should be configurable regarding the COUNT part.

Using KEYS also creates issues using Quarkus Redis with AWS ElastiCache Serverless, which does not allow the KEYS command.

E.g.: SCAN 0 MATCH cache:...* COUNT x

@holomekc holomekc added the kind/enhancement New feature or request label Jan 22, 2025
Copy link

quarkus-bot bot commented Jan 22, 2025

/cc @Ladicek (redis), @cescoffier (redis), @gwenneg (cache), @machi1990 (redis)

@Ladicek
Copy link
Contributor

Ladicek commented Jan 22, 2025

The KEYS command is only used to implement explicit invalidation, as far as I can tell. I'm not sure how often that is used, but probably not terribly often. That said, using SCAN would clearly be better than KEYS. There is higher risk of a race, but that risk is already present, so there wouldn't be a semantic change.

@holomekc
Copy link
Contributor Author

holomekc commented Jan 22, 2025

Sorry. I should have mentioned that. Yes when we use @InvalidateAll as you said this happens.
We can actually work around it, but I though I should at least mention it here.

@Ladicek
Copy link
Contributor

Ladicek commented Jan 23, 2025

Ah I didn't notice that this API is also used for annotation-based invalidation. That is of course a lot more often used. I'll take a look today, shouldn't be a big deal to fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants