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

SRANDMEMBER returns a list from kvrocks when it returns a string on redis #2610

Open
1 of 2 tasks
Rafiot opened this issue Oct 18, 2024 · 6 comments
Open
1 of 2 tasks
Labels
bug type bug

Comments

@Rafiot
Copy link

Rafiot commented Oct 18, 2024

Search before asking

  • I had searched in the issues and found no similar issues.

Version

Ubuntu 24.04 / kvrocks 2.10 / valkey 8.0

Minimal reproduce step

from redis import Redis
r.sadd('foo', 'bar')
r.srandmember('foo')

# returns [b'bar'] from kvrocks, b'bar' from valkey

What did you expect to see?

The same output against both backends.

What did you see instead?

[b'bar'] from kvrocks, b'bar' from valkey

Anything Else?

I cannot say who's wrong, it's just super confusing to have two different responses.

The solution I'll go for to have a consistent output is to pass r.srandmember('foo', 1) which returns a list against both backends.

Are you willing to submit a PR?

  • I'm willing to submit a PR!
@Rafiot Rafiot added the bug type bug label Oct 18, 2024
Rafiot added a commit to Lookyloo/lookyloo that referenced this issue Oct 18, 2024
@git-hulk
Copy link
Member

@Rafiot Thanks for your report. Redis did have a different reply behavior when count is 1.

@Rafiot
Copy link
Author

Rafiot commented Oct 19, 2024

yep. I don't know if it is something to change or not, I don't see a good solution to that, as you will either break existing kvrocks-only tools in a very weird way, or keep the difference.

It would probably make sense to document the difference somewhere so people don't spend hours debugging it, and make sure to always pass the count parameter.

@mapleFU
Copy link
Member

mapleFU commented Oct 19, 2024

I'm ok with changing this to same as redis 🤔

@git-hulk
Copy link
Member

Yes, we can fix it to align with Redis.

@Rafiot
Copy link
Author

Rafiot commented Oct 19, 2024

That's an option, but it will break existing codebases:

from redis import Redis
r.sadd('foo', 'bar')
if member := r.srandmember('foo'):
    print(member[0])
    # do something with member[0]

That call was getting the one element from the list and will now get the first char of the response, and it won't even fail.

@git-hulk
Copy link
Member

@Rafiot Good point. It'd be better to document explicitly instead of silently breaking old behaviors in some programming languages. And always return the array format makes sense whether the count is bigger than 1 or not.

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

No branches or pull requests

3 participants