Adds support for scripting engines as Valkey modules #1277
Conversation
|
I'm opening this PR in draft mode because I still need to fix test failures, and add new tests as well. |
rjd15372
force-pushed
the
engine-api-1261
branch
4 times, most recently
from
ea6e6b6
to
d3293dd
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1277 +/- ##
============================================
+ Coverage 70.55% 70.70% +0.14%
============================================
Files 115 115
Lines 63158 63231 +73
============================================
+ Hits 44561 44707 +146
+ Misses 18597 18524 -73
|
rjd15372
force-pushed
the
engine-api-1261
branch
2 times, most recently
from
4a2e223
to
1a808b2
Compare
|
I just realized I still need to implement the |
|
Core team said we are directionally aligned with, we still want someone to take a deeper look to make sure the details makes sense. |
rjd15372
force-pushed
the
engine-api-1261
branch
5 times, most recently
from
fe56090
to
9d4b296
Compare
|
The PR is ready for review. |
There was a problem hiding this comment.
This looks like great progress. Is it ready for a proper review or are you going to change much more?
Shall we do EVAL as a follow-up PR?
Each engine has only one context which is used for all calls, right? Would it be possible for an engine to use a context (or sub-context, etc.) for each client? This idea came up in the LuaJIT discussion by @secwall in #1229. A separate Lua context per client would isolate each client from each other, which compensates for the fact that Lua isn't completely sandboxed by design. Maybe the engine can organize this by itself, if the engine just has access to the current client at the time of a FCALL or EVAL?
It's ready. I don't think I'll add more changes apart from the changes asked by the reviewers.
Yes, that's the plan.
I'll take a look if it's possible, and will open a follow-up PR with the changes. |
This commit extends the module API to support the addition of different scripting engines to run user defined functions. The scripting engine can be implemented as a Valkey module, and can be dynamically loaded with the `loadmodule` config directive, or with the `MODULE LOAD` command. The current module API support, only allows to load scripting engines to run functions using `FCALL` command. In a follow up PR, we will move the Lua scripting engine implmentation into its own module. Signed-off-by: Ricardo Dias <[email protected]>
rjd15372
force-pushed
the
engine-api-1261
branch
from
9d4b296
to
fc03df7
Compare
This commit adds a module with a very simple stack based scripting language implementation to test the new module API that allows to implement new scripting engines as modules. Signed-off-by: Ricardo Dias <[email protected]>
rjd15372
force-pushed
the
engine-api-1261
branch
from
fc03df7
to
9c618a3
Compare
|
Can you avoid force-pushing? It's easier to review what's changed since last time if you just push small commits. You can even merge unstable to it, rather than rebase. We squash-merge PRs anyway in the end and we use the PR title and description as the commit message. |
Ah sorry. I wasn't aware that we were squashing the commits upon merge. I'll stop force-pushing. |
Signed-off-by: Ricardo Dias <[email protected]>
|
Today I added more test cases to test the error code paths. |
Signed-off-by: Ricardo Dias <[email protected]>
Signed-off-by: Ricardo Dias <[email protected]>
…ORE|DELETE] Signed-off-by: Ricardo Dias <[email protected]>
| if (luaL_loadbuffer(lua, blob, sdslen(blob), "@user_function")) { | ||
| *err = sdscatprintf(sdsempty(), "Error compiling function: %s", lua_tostring(lua, -1)); | ||
| if (luaL_loadbuffer(lua, blob, strlen(blob), "@user_function")) { | ||
| *err = valkey_asprintf("Error compiling function: %s", lua_tostring(lua, -1)); |
There was a problem hiding this comment.
What is the benefit of valkey_asprintf over sdscatprintf? err has been traditionally an sds string. It is not interchangeable with the normal pointer. This change would add cognitive load so I would like to better understand the rationale behind this change.
This PR extends the module API to support the addition of different scripting engines to execute user defined functions.
The scripting engine can be implemented as a Valkey module, and can be dynamically loaded with the
loadmoduleconfig directive, or with theMODULE LOADcommand.This PR also adds an example of a dummy scripting engine module, to show how to use the new module API. The dummy module is implemented in
tests/modules/helloscripting.c.The current module API support, only allows to load scripting engines to run functions using
FCALLcommand.In a follow up PR, we will move the Lua scripting engine implmentation into its own module.