kvrocks icon indicating copy to clipboard operation
kvrocks copied to clipboard

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

Open ShooterIT opened this issue 3 years ago • 13 comments

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:

  • Support to persist all lua scripts in Kvrocks, so scripts is not client cache
  • Support to replicate lua scripts between master and replicas
  • Also support to replicate the change of executing lua scripts

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.

ShooterIT avatar Feb 16 '22 01:02 ShooterIT

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

  • For creating, when the name length was equal to 40 chars, use it directly as raw function name, or hash(name) to fixed 40 chars.
  • For looking, when the name length was 40, use it as key directly, or use hash(name) as key.

git-hulk avatar Feb 21 '22 10:02 git-hulk

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 avatar Feb 24 '22 09:02 ShooterIT

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

ShooterIT avatar Feb 24 '22 09:02 ShooterIT

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.

git-hulk avatar Feb 24 '22 15:02 git-hulk

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

ShooterIT avatar Feb 25 '22 01:02 ShooterIT

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?

taylorchu avatar Jun 02 '22 19:06 taylorchu

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

git-hulk avatar Jun 03 '22 00:06 git-hulk

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

taylorchu avatar Jun 03 '22 04:06 taylorchu

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.

git-hulk avatar Jun 03 '22 04:06 git-hulk

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.

FZambia avatar Dec 02 '22 06:12 FZambia

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.

git-hulk avatar Dec 02 '22 07:12 git-hulk

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

FZambia avatar Dec 02 '22 07:12 FZambia

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

git-hulk avatar Dec 02 '22 07:12 git-hulk