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.54k stars 465 forks source link

[NEW] Support to call Lua script by name like stored procedures #485

Closed ShooterIT closed 1 month ago

ShooterIT commented 2 years ago

The problem/use-case that the feature addresses

Redis issue: https://github.com/redis/redis/issues/8693 Redis PR: https://github.com/redis/redis/pull/9780

Provide a way to make scripts similar with stored procedures, it is not convenient to use SHA as a function name to call

Description of the feature

Current status:

So the most important things is that we provide a way to set lua script name and call it by its name.

Alternatives you've considered

NULL

Additional information

Don't need to support multi engines, since currently for most users, lua script is enough, importantly, we should provide a mechanism like stored procedures

We need to make this mechanism compatible with lua script implementation, don't break old usages.

git-hulk commented 2 years ago

I have an idea for this issue that we can recognize SHA or literal name with the name length.

ShooterIT commented 2 years ago

How to set scripts name

We can set scripts name when calling SCRIPTS LOAD. SCRIPTS LOAD $scripts [NAME $name]

Also we can support set sha of scripts to new name SCRIPTS SETNAME $name SHA $scripts_sha

Also can support rename of scripts name SCRIPTS SETNAME $name NAME $scripts_name

How to call scripts by name

New command to call scripts by name EVALCALL $scripts_name(can 40 char)

We call can support call scripts by name by EVALSHA, but name MUST NOT be 40 char EVALSHA $name(must not 40 char)

ShooterIT commented 2 years ago

also can set script name in EVAL command change EVAL script numkeys key [key …] arg [arg …] into EVAL script [NAME $name] numkeys key [key …] arg [arg …]

in EVAL command, numkeys(a number) always is necessary, so we can distinguish [NAME $name] with numkeys

git-hulk commented 2 years ago

cool, I think we don't need to limit the length of name. We can still use the fixed 40 chars as script key name, and the name if not 40 chars or use it as key directly. So we can determine whether need to hash the script name or not before lookup script.

ShooterIT commented 2 years ago

We call can support call scripts by name by EVALSHA, but name MUST NOT be 40 char EVALSHA $name(must not 40 char)

oh, yes, can be 40 char, but we should search sha firstly, and if not find, then call it as name

taylorchu commented 2 years ago

ERR NOSCRIPT No matching script. Please use EVAL

while I try to integrate kvrocks with existing redis integration tests, I see this error. does kvrocks currently store the lua scripts?

git-hulk commented 2 years ago

@taylorchu Yes, Kvrocks does store and propagate scripts now, what errors you occurred, can you paste the error or reproduce steps?

taylorchu commented 2 years ago

it is a protocol-related issue from lua script from calling evalsha:

the application will first use evalsha to see if script is cached. if it is not cached, then retry with eval. however the library I used (go-redis) requires NOSCRIPT error string to be formatted in certain way.

redis: NOSCRIPT No matching script. Please use EVAL. kvrocks: ERR NOSCRIPT No matching script. Please use EVAL

git-hulk commented 2 years ago

Oooop, sorry to hear that. We should keep those messages consistent as possible as we can. You can help to submit a issue and PR is always welcomed.

FZambia commented 1 year ago

Also came across the problem mentioned in https://github.com/apache/incubator-kvrocks/issues/485#issuecomment-1145563442 with another library in Go ecosystem - it's not possible to use Lua in kvrocks. I looked through the code where NOSCRIPT is used in kvrocks but have not found obvious way to fix this.

git-hulk commented 1 year ago

Hi @FZambia go-redis has compatible with this error message for Kvrocks, can see error.go#L23.

So you can upgrade your library to https://github.com/go-redis/redis/releases/tag/v9.0.0-rc.2, or copy out the Run and HasErrorPrefix functions to your codebase if you don't wanna do that.

I'm very surprised and grateful for the help from @vmihailenco.

FZambia commented 1 year ago

@git-hulk thanks, I am using another library, of course I can do any kind of hack in my final app - but just emphasising that kvrocks will have this issue with many libraries until error message will be consistent with one in Redis. There are several more Redis libraries in Go ecosystem I know outside go-redis which won't work due to the ERR prefix for noscript error.

git-hulk commented 1 year ago

@FZambia Yes, we will also fix this issue on Kvrock's side soon.

tisonkun commented 1 month ago

@git-hulk @PragmaTwice do we support this feature now? I remember we have something related to this.

PragmaTwice commented 1 month ago

Yeah Kvrocks has supported Redis Functions now.