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

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
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
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?

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

Successfully merging this pull request may close these issues.

2 participants