apache / kvrocks

Apache Kvrocks is a distributed key value NoSQL database that uses RocksDB as storage engine and is compatible with Redis protocol.
https://kvrocks.apache.org/
Apache License 2.0
3.53k stars 464 forks source link

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

Open Rafiot opened 1 week ago

Rafiot commented 1 week ago

Search before asking

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?

git-hulk commented 1 week ago

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

Rafiot commented 1 week ago

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 commented 1 week ago

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

git-hulk commented 1 week ago

Yes, we can fix it to align with Redis.

Rafiot commented 1 week ago

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 commented 1 week ago

@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.