Skip to content

feat(services/redis): add support of list operation #5304

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

Closed
wants to merge 4 commits into from

Conversation

PragmaTwice
Copy link
Member

Which issue does this PR close?

Closes #.

Rationale for this change

Currently only read and write is supported for redis service.

After #5208 we can have a better support for kv scanning, so list supporting for redis is relatively easy to implement and can be efficient.

What changes are included in this PR?

We support (async) list operation for redis service via kv::Adaptor::scan to fetch paged key lists with a prefix using Redis SCAN command.

Are there any user-facing changes?

User now can perform list on redis schema.

@PragmaTwice PragmaTwice requested a review from Xuanwo as a code owner November 10, 2024 06:34
@github-actions github-actions bot added the releases-note/feat The PR implements a new feature or has a title that begins with "feat" label Nov 10, 2024
@PragmaTwice
Copy link
Member Author

Funny to see it succeeded on redis but failed on redis cluster and kvrocks.

@PragmaTwice PragmaTwice requested a review from PsiACE as a code owner November 11, 2024 04:30
@PragmaTwice
Copy link
Member Author

Fixed.


unsafe impl Sync for RedisScanner {}

impl Stream for RedisScanner {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, how about implement kv::Scan directly without go through Stream?


#[self_referencing]
pub struct RedisScanner {
pool: bb8::Pool<RedisConnectionManager>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could extract RedisCore as other services do and use Arc<RedisCore> here to avoid duplicate calls like conn_from_pool.

@@ -336,6 +387,12 @@ impl kv::Adapter for Adapter {
Capability {
read: true,
write: true,
// due to limitation of Redis itself,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this is the first time I've learned this. Would you like to add a note in the documents about this backend to make it clear to users?

@Xuanwo
Copy link
Member

Xuanwo commented Apr 7, 2025

Hi @PragmaTwice, I'm working on #5739 now. And I'm guessing this PR won't be needed anymore after that. Thank you for your working!

@Xuanwo Xuanwo closed this Apr 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
releases-note/feat The PR implements a new feature or has a title that begins with "feat"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants