Additional AOFRW deprecation cleanups #9794
Comments
# Redis Function This PR added the Redis Functions capabilities that were suggested on #8693. The PR also introduce a big refactoring to the current Lua implementation (i.e `scripting.c`). The main purpose of the refactoring is to have better code sharing between the Lua implementation that exists today on Redis (`scripting.c`) and the new Lua engine that is introduced on this PR. The refactoring includes code movements and file name changes as well as some logic changes that need to be carefully reviewed. To make the review easier, the PR was split into multiple commits. Each commit is deeply described later on but the main concept is that some commits are just moving code around without making any logical changes, those commits are less likely to cause any issues or regressions and can be reviewed fast. Other commits, which perform code and logic changes, need to be reviewed carefully, but those commits were created after the code movements so it's pretty easy to see what was changed. To sum up, it is highly recommended to review this PR commit by commit as it will be easier to see the changes, it is also recommended to read each commit description (written below) to understand what was changed on the commit and whether or not it's just a huge code movement or a logic changes. ## Terminology Currently, the terminology in Redis is not clearly defined. Scripts refer to Lua scripts and eval also refers only to Lua. Introducing Redis Function requires redefining those terms to be able to clearly understand what is been discussed on each context. * eval - legacy Lua script implementation. * Function - new scripting implementation (currently implemented in Lua but in the future, it might be other languages like javascript). * Engine - the component that is responsible for executing functions. * Script - Function or legacy Lua (executed with `eval` or `evalsha`) ## Refactoring New Structure Today, the entire scripting logic is located on `scripting.c`. This logic can be split into 3 main groups: 1. Script management - responsible for storing the scripts that were sent to Redis and retrieving them when they need to be run (base on the script sha on the current implementation). 2. Script invocation - invoke the script given on `eval` or `evalsha` command (this part includes finding the relevant script, preparing the arguments, ..) 3. Interact back with Redis (command invocation) Those 3 groups are tightly coupled on `scripting.c`. Redis Functions also need to use those groups logics, for example, to interact back with Redis or to execute Lua code. The refactoring attempts to split those 3 groups and define APIs so that we can reuse the code both on legacy Lua scripts and Redis Functions. In order to do so we define the following units: 1. script.c: responsible for interaction with Redis from within a script. 2. script_lua.c: responsible to execute Lua code, uses `script.c` to interact with Redis from within the Lua code. 3. function_lua.c: contains the Lua engine implementation, uses `script_lua.c` to execute the Lua code. 4. functions.c: Contains Redis Functions implementation (`FUNCTION` command,), uses `functions_lua.c` if the function it wants to invoke needs the Lua engine. 4. eval.c: the original `scripting.c` contains the Lua legacy implementation and was refactored to use `script_lua.c` to invoke the Lua code. ## Commits breakdown Notice: Some small commits are omitted from this list as they are small and insignificant (for example build fixes) ### First commit - code movements This commit rename `scripting.c` -> `eval.c` and introduce the new `script_lua.c` unit. The commit moves relevant code from `eval.c` (`scripting.c`) to `script_lua.c`, the purpose of moving the code is so that later we will be able to re-use the code on the Lua engine (`function_lua.c`). The commit only moves the code without modifying even a single line, so there is a very low risk of breaking anything and it also makes it much easier to see the changes on the following commits. Because the commit does not change the code (only moves it), it does not compile. But we do not care about it as the only purpose here is to make the review processes simpler. ### Second commit - move legacy Lua variables into `eval.c` Today, all Lua-related variables are located on the server struct. The commit attempt to identify those variable and take them out from the server struct, leaving only script related variables (variables that later need to be used also by engines) The following variable where renamed and left on the server struct: * lua_caller -> script_caller * lua_time_limit -> script_time_limit * lua_timedout -> script_timedout * lua_oom -> script_oom * lua_disable_deny_script -> script_disable_deny_script * in_eval -> in_script The following variables where moved to lctx under eval.c * lua * lua_client * lua_cur_script * lua_scripts * lua_scripts_mem * lua_replicate_commands * lua_write_dirty * lua_random_dirty * lua_multi_emitted * lua_repl * lua_kill * lua_time_start * lua_time_snapshot This commit is in a low risk of introducing any issues and it is just moving variables around and not changing any logic. ### Third commit - introducing script unit This commit introduces the `script.c` unit. Its purpose (as described above) is to provide an API for scripts to interact with Redis. Interaction includes mostly executing commands, but also other functionalities. The interaction is done using a `ScriptRunCtx` object that needs to be created by the user and initialized using `scriptPrepareForRun`. A detailed list of functionalities expose by the unit: 1. Calling commands (including all the validation checks such as acl, cluster, read only run, ...) 2. Set Resp 3. Set Replication method (AOF/REPLICATION/NONE) 4. Call Redis back on long-running scripts to allow Redis to reply to clients and perform script kill The commit introduces the new unit and uses it on eval commands to interact with Redis. ### Fourth commit - Moved functionality of invoke Lua code to `script_lua.c` This commit moves the logic of invoking the Lua code into `script_lua.c` so later it can be used also by Lua engine (`function_lua.c`). The code is located on `callFunction` function and assumes the Lua function already located on the top of the Lua stack. This commit also change `eval.c` to use the new functionality to invoke Lua code. ### Fith commit - Added Redis Functions unit (`functions.c`) and Lua engine (`function_lua.c`) Added Redis Functions unit under `functions.c`, included: 1. FUNCTION command: * FUNCTION CREATE * FUNCTION CALL * FUNCTION DELETE * FUNCTION KILL * FUNCTION INFO * FUNCTION STATS 2. Register engines In addition, this commit introduces the first engine that uses the Redis Functions capabilities, the Lua engine (`function_lua.c`) ## API Changes ### `lua-time-limit` configuration was renamed to `script-time-limit` (keep `lua-time-limit` as alias for backward compatibility). ### Error log changes When integrating with Redis from within a Lua script, the `Lua` term was removed from all the error messages and instead we write only `script`. For example: `Wrong number of args calling Redis command From Lua script` -> `Wrong number of args calling Redis command From script` ### `info memory` changes: Before stating all the changes made to memory stats we will try to explain the reason behind them and what we want to see on those metrics: * memory metrics should show both totals (for all scripting frameworks), as well as a breakdown per framework / vm. * The totals metrics should have "human" metrics while the breakdown shouldn't. * We did try to maintain backward compatibility in some way, that said we did make some repurpose to existing metrics where it looks reasonable. * We separate between memory used by the script framework (part of redis's used_memory), and memory used by the VM (not part of redis's used_memory) A full breakdown of `info memory` changes: * `used_memory_lua` and `used_memory_lua_human` was deprecated, `used_memory_vm_eval` has the same meaning as `used_memory_lua` * `used_memory_scripts` was renamed to `used_memory_scripts_eval` * `used_memory_scripts` and `used_memory_scripts_human` were repurposed and now return the total memory used by functions and eval (not including vm memory, only code cache, and structs). * `used_memory_vm_function` was added and represents the total memory used by functions vm's * `used_memory_functions` was added and represents the total memory by functions (not including vm memory, only code cache, and structs) * `used_memory_vm_total` and `used_memory_vm_total_human` was added and represents the total memory used by vm's (functions and eval combined) ### `functions.caches` `functions.caches` field was added to `memory stats`, representing the memory used by engines that are not functions (this memory includes data structures like dictionaries, arrays, ...) ## New API ### FUNCTION CREATE Usage: FUNCTION CREATE `ENGINE` `NAME` `[REPLACE]` `[DESC <DESCRIPTION>]` `<CODE>` * `ENGINE` - The name of the engine to use to create the script. * `NAME` - the name of the function that can be used later to call the function using `FUNCTION CALL` command. * `REPLACE` - if given, replace the given function with the existing function (if exists). * `DESCRIPTION` - optional argument describing the function and what it does * `CODE` - function code. The command will return `OK` if created successfully or error in the following cases: * The given engine name does not exist * The function name is already taken and `REPLACE` was not used. * The given function failed on the compilation. ### FCALL and FCALL_RO Usage: FCALL/FCALL_RO `NAME` `NUM_KEYS key1 key2` … ` arg1 arg2` Call and execute the function specified by `NAME`. The function will receive all arguments given after `NUM_KEYS`. The return value from the function will be returned to the user as a result. * `NAME` - Name of the function to run. * The rest is as today with EVALSHA command. The command will return an error in the following cases: * `NAME` does not exist * The function itself returned an error. The `FCALL_RO` is equivalent to `EVAL_RO` and allows only read-only commands to be invoked from the script. ### FUNCTION DELETE Usage: FUNCTION DELETE `NAME` Delete a function identified by `NAME`. Return `OK` on success or error on one of the following: * The given function does not exist ### FUNCTION INFO Usage: FUNCTION INFO `NAME` [WITHCODE] Return information about a function by function name: * Function name * Engine name * Description * Raw code (only if WITHCODE argument is given) ### FUNCTION LIST Usage: FUNCTION LIST Return general information about all the functions: * Function name * Engine name * Description ### FUNCTION STATS Usage: FUNCTION STATS Return information about the current running function: * Function name * Command that was used to invoke the function * Duration in MS that the function is already running If no function is currently running, this section is just a RESP nil. Additionally, return a list of all the available engines. ### FUNCTION KILL Usage: `FUNCTION KILL` Kill the currently executing function. The command will fail if the function already initiated a write command. ## Notes Note: Function creation/deletion is replicated to AOF but AOFRW is not implemented sense its going to be removed: #9794
|
For the record. we discussed these in a core-team meeting. however, we concluded that we don't want to delete the code in rewriteAppendOnlyFileRio and deprecate the preamble config yet. |
|
So, I understand that we gave up these plans, right? |
|
as far as i'm aware, we only gave up the first two bullets on the top, we do wanna implement the 3rd, and maybe the 4th too. |
|
@oranagra This is what I understand and support as well. |
|
discussed this again in the core-team meeting. decided to proceed with items 3 and 4 in the top list |
|
I think this needs more detailed description, or I will mention a draft separately when I have time. |
|
number 4 seems quite straight forward. |
|
several questions:
|
|
i don't recall if this was already specified or not, so i'll just respond instead of trying to find it. the main reasons for always generating a base file are:
This feature of generating a base file when starting up empty isn't needed if preamble is disabled IMHO. regarding disallowing BGREWRITEAOF, i think it complicates redis (IIRC we saw 2 bugs recently because of that feature), and i don't see why that feature is needed. so i thought it's a good idea to remove it. |
|
@oranagra Do you mean that when redis starts and finds that base does not exist, it generates an RDB in the foreground and then loads it? IIRC the previous AOF mechanism did not do this? The preamble is written to the AOF only after the first AOFRW. |
|
I meant that if redis starts configured for |
|
yes, the previous AOF (with preamble rdb header) didn't have that mechanism, but in theory it could be added there too (if we wanted to "improve" redis 6.2) |
|
If we only have a INCR aof when redis starts, do we need to force create a BASE ? |
no, only create an empty base file if nothing was loaded on startup. |
|
I got to know some Chinese cloud companies. They allow customers to use |
|
I think it's better to allow If you want more detailed examples, here is a document we (Alibaba cloud) help people to use aof files to migrate data, https://www.alibabacloud.com/help/en/doc-detail/26357.html , in this doc we suggest users open aof to migrate data, but as I know some users still like close appendonly and just use |
|
Another if you want: https://support.huaweicloud.com/intl/en-us/migration-dcs/dcs-migration-0312010.html |
|
Thank you. so considering all of that, do you really think blocking BGREWRITEAOF in that scenario would be a breaking change in 7.0? p.s. i still didn't give up the desire to completely delete the |
|
@oranagra May be you right, but i think we can't give up compatibility just because a thing will cause complexity, and at least so far I haven't found how complicated it is and what problems it will cause, if there is, please point it out, I'll be happy to solve it this matter. |
|
Also,IIRC, |
|
there's obviously no standard way to decide, we just try to do what we think is best. I suppose it's just that i wasn't aware of the use cases you're mentioning now, which is why i thought we can chuck it. so the advise in these links doesn't seem very useful, and maybe we should come up with a different solution for these use cases in the future. After this discussion, I guess i don't mind closing this issue and leaving that thing as is. but i must say i don't see a bright future for this "feature", the use case for it is better served in other ways. |
|
regarding your last post about proto-max-bulk-len and client-query-buffer-limit and the issue with a huge RESTORE command. that's a known issue and the roadmap has other solution for it. |
|
Wow, great to see this feature in RESP3. |
|
i maintained a similar tool in Redis Labs, i'm well aware of the limitations of redis and the complications of converting RDB file to RESTORE commands. |
I wish we don't break this change, it is charismatic for redis to provide many methods to use freely. IIRC, some users actually use this feature 😂, AOF is easy to inject to another running redis server, such as by I also agree AOF is a convenient method to backup, actually we ever used this way instead of RDB since RDB didn't support incrementally |
|
ok... i'm ok with dropping this change from 7.0. |
oranagra
added
state:to-be-closed
oranagra
removed
the
state:to-be-closed
oranagra
changed the title
always generate base rdb (even when starting empty)(done in Always create base AOF file when redis start from empty. #10102)details: #9539 (comment)
The text was updated successfully, but these errors were encountered: