Removing Redis from internal lua function names and comments by parthpatel · Pull Request #288 · valkey-io/valkey

parthpatel · 2024-04-10T21:56:27Z

Fixed a bug where 'server *' invocation from lua debugger was not working. Manually tested it as follows -

Before 

lua debugger> redis ping
<redis> ping
<reply> "+PONG"
lua debugger> server ping
<error> Unknown Redis Lua debugger command or wrong number of arguments.

After

% ./src/valkey-cli --ldb --eval /tmp/1
* Stopped at 1, stop reason = step over
-> 1   return redis.call('ping');
lua debugger> server ping
<redis> ping
<reply> "+PONG"

Functions in lua c files lack of documentation. Adding some documentation in this commit.
I removed Redis from function names in eval.c and script_lua.c files. I did not touch log messages in this commit. I also did not touch references to "RedisProtocol".

madolson · 2024-04-10T23:10:08Z

    SERVER_API_NAME,
    REDIS_API_NAME,
-    "__redis__err__handler", /* error handler for eval, currently located on globals.
+    "__server__err__handler", /* error handler for eval, currently located on globals.


I'm not clear if this is a breaking change or not. I was fine just leaving it as redis for now. Since we want to backport this, I would like this PR to represent what we backport.

From what I understand, this is only used to handle errors internally. It is not to be invoked by users anywhere. I changed it in both places, where it is registered and where it gets invoked from.

People have complained about us touching globals in the past, even ones they weren't supposed to be using. I'm not sure if anyone would materially call this, but I would rather lean on the conservative side and not touch it.

Google search returned just one hit on "__redis__err__handler" so I guess the risk is very low here. I think we can also consider aliasing __server__err__handler to __redis__err__handler when defining the handler function, if we are concerned about compat.

I will alias them. If I can't then I will duplicate them.

@madolson Tables are definitely aliased because lua uses a reference for them. That's how, all the fields added after luaRegisterServerAPI to 'server' table magically appear under 'redis' table as well.

madolson · 2024-04-10T23:11:52Z


-    /* register debug commands */
-    lua_getglobal(lua,"redis");
+    /* register debug commands. we only need to add it under 'server' as 'redis' is effectively aliased to 'server' table at this point. */


Suggested change

/* register debug commands. we only need to add it under 'server' as 'redis' is effectively aliased to 'server' table at this point. */

/* register debug commands. We only need to add it under 'server' as 'redis' will be created as a copy of server later. */

I think this is closer to how it works.

We also have 120 line length, and I think this barely spill over that.

yeah, I was using Vim. I need to set this up in IntelliJ, it automatically splits larger lines. I will fix it for this PR manually.

Let's not start a war about this but I believe antirez style is 80 chars width. That's also the standard for terminals. (Open up one a new terminal window you'll see.)

For multi-line comments I think it's nice to keep it around 80. It's easier to read. For long code lines, we can have a soft limit around 120 imo. (Let's not start a war about it though.)

(In emacs press Alt-Q inside a comment to reflow it.)

Yeah. The style the old Redis maintainers used was 120 max length, soft limit around 100, 80 encouraged for multi-line paragraphs with a lot of breaks. The only hard limit was 120.

clang-format is still on my to-do list. After the "rebranding" work (which pretty much makes the code history discussion moot), I think we should just get out of the style discussion altogether and let the "machine" do the work automatically.

Also a nitpick. This comment "we only need to add it under 'server' as 'redis' is effectively aliased to 'server' table at this point" seems to be more relevant to this PR than something we like to keep in the code for a longer time. The reference to the "server" table is pretty clear to me already. The aliasing of "server" to "redis" is a hack that's better be kept in the smallest possible scope.

@PingXie I did not understand your suggestion. Do you want to modify the comment or remove it altogether? I like to have documentation around any code - temporary or not. We can remove the documentation when we remove the code - in this case 'redis' table from lua.

Sorry I meant to say that we don't have to mention that "redis" and "server" are synonymous since that is covered in other part of the code already (like when you set up the "redis" alias for the "server" table).

madolson · 2024-04-10T23:14:14Z

+    /* Replace math.random and math.randomseed with custom implementations. */
    lua_getglobal(lua,"math");
-
+    /* Add random and randomseed functions. */


Suggested change

/* Add random and randomseed functions. */

I feel like this comment isn't saying anything, but I see similar comments exist throughout this function. So don't feel that strongly about it.

yeah there were many such comments, so instead of removing I just made them a full sentence.

madolson

Generally, can you split this into two PRs.

Strictly what we need to backport.
Everything else. I think all your suggestions are pretty good, they will just be a bit annoying to backport since they are so widespread.

parthpatel · 2024-04-11T02:37:26Z

Generally, can you split this into two PRs.

Strictly what we need to backport.

Everything else. I think all your suggestions are pretty good, they will just be a bit annoying to backport since they are so widespread.

Agreed. I will split it, so that the bugfix is easier to backport.

PingXie · 2024-04-11T18:48:23Z

-void ldbLogRedisReply(char *reply) {
+void ldbLogServerReply(char *reply) {
    sds log = sdsnew("<reply> ");
    ldbRedisProtocolToHuman(&log,reply);


There are still a few "Redis" references in this file. Can we fix them all at once?

Suggested change

ldbRedisProtocolToHuman(&log,reply);

ldbServerProtocolToHuman(&log,reply);

I thought we decided to keep references to RedisProtocol? Are we renaming the protocol from RESP to something else?

The protocol is not called "server protocol". It doesn't say much.

I suggested earlier (another PR) that we keep it as RedisProtocol here.

If we insist on changing it, we can change Redis to RESP here, i.e. name the function to ldbRespToHuman or ldbRespProtocolToHuman.

Yeah, I would vote updating it to RESP, which feels more accurate.

PingXie · 2024-04-11T18:50:39Z

    SERVER_API_NAME,
    REDIS_API_NAME,
-    "__redis__err__handler", /* error handler for eval, currently located on globals.
+    "__server__err__handler", /* error handler for eval, currently located on globals.


Google search returned just one hit on "__redis__err__handler" so I guess the risk is very low here. I think we can also consider aliasing __server__err__handler to __redis__err__handler when defining the handler function, if we are concerned about compat.

PingXie · 2024-04-11T18:52:00Z

+    lua_pushcfunction(lua,server_math_randomseed);
    lua_settable(lua,-3);
-
+    /* overwrite value of global variable 'math' with this modified math table present on top of stack. */


nit - this line is pretty obvious IMO so probably not worth a code comment

PingXie · 2024-04-11T18:54:55Z

+    /* In addition to registering server.call/pcall API, we will throw a customer message when a script accesses undefined global variable. 
+     * LUA stored global variables in the global table, accessible to us on stack at virtual index = LUA_GLOBALSINDEX. 
+     * We will set __index handler in global table's metatable to a custom C function to achieve this - handled by luaSetAllowListProtection.  
+     * Refer to https://www.lua.org/pil/13.4.1.html for documentation on __index and https://www.lua.org/pil/contents.html#13 for documentation on metatables.  
+     * We need to pass global table to lua invocations as parameters. To achieve this, lua_pushvalue invocation brings global variable table to the top of 
+     * the stack by pushing value from global index onto the stack. And lua_pop invocation after luaSetAllowListProtection removes it - resetting the stack to its original state. */ 


The comment LGTM.

I haven't counted but this seems going beyond 120 chars?

yeah fixed it. There are still a lot of existing code lines in the file going beyond 120 characters, so we will need a more automated cleanup.

PingXie · 2024-04-11T20:11:16Z


-    /* register debug commands */
-    lua_getglobal(lua,"redis");
+    /* register debug commands. we only need to add it under 'server' as 'redis' is effectively aliased to 'server' table at this point. */


clang-format is still on my to-do list. After the "rebranding" work (which pretty much makes the code history discussion moot), I think we should just get out of the style discussion altogether and let the "machine" do the work automatically.

PingXie · 2024-04-11T20:16:48Z


-    /* register debug commands */
-    lua_getglobal(lua,"redis");
+    /* register debug commands. we only need to add it under 'server' as 'redis' is effectively aliased to 'server' table at this point. */


Also a nitpick. This comment "we only need to add it under 'server' as 'redis' is effectively aliased to 'server' table at this point" seems to be more relevant to this PR than something we like to keep in the code for a longer time. The reference to the "server" table is pretty clear to me already. The aliasing of "server" to "redis" is a hack that's better be kept in the smallest possible scope.

PingXie · 2024-04-11T20:20:35Z

        } else if (argc > 1 &&
-                   (!strcasecmp(argv[0],"r") || !strcasecmp(argv[0],"redis"))) {
-            ldbRedis(lua,argv,argc);
+                   (!strcasecmp(argv[0],"r") || !strcasecmp(argv[0],"redis") || !strcasecmp(argv[0],"server"))) {


another nipick - should we consolidate all "redis"/"server" table references on "SERVER_API_NAME" and "REDIS_API_NAME"? since we have done the (indirection) work, might just continue with the pattern?

PingXie · 2024-04-11T20:49:49Z

+static int server_math_random (lua_State *L) {
  /* the `%' avoids the (rare) case of r==1, and is needed also because on
     some systems (SunOS!) `rand()' may return a value larger than RAND_MAX */
  lua_Number r = (lua_Number)(serverLrand48()%REDIS_LRAND48_MAX) /


Rename REDIS_LRAND48_MAX?

Suggested change

lua_Number r = (lua_Number)(serverLrand48()%REDIS_LRAND48_MAX) /

lua_Number r = (lua_Number)(serverLrand48()%VALKEY_LRAND48_MAX) /

good catch. done.

…all invocations. (#303) * Tested it on local instance. This was originally part of #288 but I am pushing this separately, so that we can easily merge it into the upcoming release. ``` lua debugger> server ping <redis> ping <reply> "+PONG" lua debugger> redis ping <redis> ping <reply> "+PONG" ``` * I also searched for lua debugger related unit tests to add coverage for this but did not find any relevant test to modify. Leaving it at that for now. --------- Signed-off-by: Parth Patel <661497+parthpatel@users.noreply.github.com>

parthpatel · 2024-04-11T23:00:33Z

I accidentally closed this review while resetting the branch, new to github workflows. Reopening this PR. I will take care of the comments a little later in the day.

parthpatel

Ran make and make test. Also verified that the error handler still works.

parthp@88665a1dcdcb placeholderkv % ./src/valkey-cli --eval /tmp/1
(error) ERR user_script:1: Script attempted to access nonexistent global variable 'print' script: 08fa7f3474473aaa414ca70bc376721a5d4b087f, on @user_script:1.

parthpatel · 2024-04-11T23:04:39Z


-    /* register debug commands */
-    lua_getglobal(lua,"redis");
+    /* register debug commands. we only need to add it under 'server' as 'redis' is effectively aliased to 'server' table at this point. */


@PingXie I did not understand your suggestion. Do you want to modify the comment or remove it altogether? I like to have documentation around any code - temporary or not. We can remove the documentation when we remove the code - in this case 'redis' table from lua.

parthpatel · 2024-04-11T23:06:01Z

-void ldbLogRedisReply(char *reply) {
+void ldbLogServerReply(char *reply) {
    sds log = sdsnew("<reply> ");
    ldbRedisProtocolToHuman(&log,reply);


I thought we decided to keep references to RedisProtocol? Are we renaming the protocol from RESP to something else?

parthpatel · 2024-04-11T23:11:16Z

+    /* In addition to registering server.call/pcall API, we will throw a customer message when a script accesses undefined global variable. 
+     * LUA stored global variables in the global table, accessible to us on stack at virtual index = LUA_GLOBALSINDEX. 
+     * We will set __index handler in global table's metatable to a custom C function to achieve this - handled by luaSetAllowListProtection.  
+     * Refer to https://www.lua.org/pil/13.4.1.html for documentation on __index and https://www.lua.org/pil/contents.html#13 for documentation on metatables.  
+     * We need to pass global table to lua invocations as parameters. To achieve this, lua_pushvalue invocation brings global variable table to the top of 
+     * the stack by pushing value from global index onto the stack. And lua_pop invocation after luaSetAllowListProtection removes it - resetting the stack to its original state. */ 


yeah fixed it. There are still a lot of existing code lines in the file going beyond 120 characters, so we will need a more automated cleanup.

parthpatel · 2024-04-11T23:17:00Z

+static int server_math_random (lua_State *L) {
  /* the `%' avoids the (rare) case of r==1, and is needed also because on
     some systems (SunOS!) `rand()' may return a value larger than RAND_MAX */
  lua_Number r = (lua_Number)(serverLrand48()%REDIS_LRAND48_MAX) /


good catch. done.

parthpatel · 2024-04-11T23:17:29Z

+    /* Replace math.random and math.randomseed with custom implementations. */
    lua_getglobal(lua,"math");
-
+    /* Add random and randomseed functions. */


parthpatel · 2024-04-11T23:17:34Z

+    lua_pushcfunction(lua,server_math_randomseed);
    lua_settable(lua,-3);
-
+    /* overwrite value of global variable 'math' with this modified math table present on top of stack. */


parthpatel · 2024-04-11T23:23:38Z

    SERVER_API_NAME,
    REDIS_API_NAME,
-    "__redis__err__handler", /* error handler for eval, currently located on globals.
+    "__server__err__handler", /* error handler for eval, currently located on globals.


I will alias them. If I can't then I will duplicate them.

@madolson Tables are definitely aliased because lua uses a reference for them. That's how, all the fields added after luaRegisterServerAPI to 'server' table magically appear under 'redis' table as well.

zuiderkwast · 2024-04-12T00:11:38Z

 /* Reply to client 'c' converting the top element in the Lua stack to a
 * server reply. As a side effect the element is consumed from the stack.  */
-static void luaReplyToRedisReply(client *c, client* script_client, lua_State *lua) {
+static void luaReplyToServerReply(client *c, client* script_client, lua_State *lua) {


ServerReply = RESP?

~~Maybe we lost the meaning when renaming to ServerReply? Another option could be luaReplyToRespReply.~~

Nah... I don't think it's a source of confusion. Ignore this comment. :)

…all invocations. (valkey-io#303) * Tested it on local instance. This was originally part of valkey-io#288 but I am pushing this separately, so that we can easily merge it into the upcoming release. ``` lua debugger> server ping <redis> ping <reply> "+PONG" lua debugger> redis ping <redis> ping <reply> "+PONG" ``` * I also searched for lua debugger related unit tests to add coverage for this but did not find any relevant test to modify. Leaving it at that for now. --------- Signed-off-by: Parth Patel <661497+parthpatel@users.noreply.github.com>

madolson · 2024-05-13T00:00:37Z

@parthpatel Are you still planning to work on this?

hpatro · 2025-01-06T22:55:17Z

This is a closed PR. Removing from backport for 7.2.

parthpatel requested review from madolson and zuiderkwast April 10, 2024 21:56

madolson reviewed Apr 10, 2024

View reviewed changes

PingXie reviewed Apr 11, 2024

View reviewed changes

parthpatel closed this Apr 11, 2024

parthpatel mentioned this pull request Apr 11, 2024

Fixing a lua debugger bug that prevented use of 'server' for server.call invocations. #303

Merged

parthpatel reopened this Apr 11, 2024

parthpatel commented Apr 11, 2024

View reviewed changes

zuiderkwast reviewed Apr 12, 2024

View reviewed changes

Comment thread src/script_lua.c Outdated

madolson changed the title ~~Fixed a bug in lua debugger for 'server *' invocations and Removing Redis from internal function names.~~ Removing Redis from internal lua function names and comments Apr 14, 2024

parthpatel closed this Oct 1, 2024

zuiderkwast mentioned this pull request Oct 1, 2024

Removing Redis from internal lua function names and comments #1102

Merged

	/* register debug commands. we only need to add it under 'server' as 'redis' is effectively aliased to 'server' table at this point. */
	/* register debug commands. We only need to add it under 'server' as 'redis' will be created as a copy of server later. */

	ldbRedisProtocolToHuman(&log,reply);
	ldbServerProtocolToHuman(&log,reply);

	lua_Number r = (lua_Number)(serverLrand48()%REDIS_LRAND48_MAX) /
	lua_Number r = (lua_Number)(serverLrand48()%VALKEY_LRAND48_MAX) /

Uh oh!

Conversation

parthpatel commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

madolson Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

parthpatel commented Apr 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parthpatel commented Apr 11, 2024

Uh oh!

parthpatel commented Apr 10, 2024 •

edited

Loading

madolson Apr 10, 2024 •

edited

Loading

zuiderkwast Apr 12, 2024 •

edited

Loading