Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing Redis from internal lua function names and comments #288

Open
wants to merge 3 commits into
base: unstable
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
62 changes: 31 additions & 31 deletions src/eval.c
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ struct ldbState {
int bp[LDB_BREAKPOINTS_MAX]; /* An array of breakpoints line numbers. */
int bpcount; /* Number of valid entries inside bp. */
int step; /* Stop at next line regardless of breakpoints. */
int luabp; /* Stop at next line because redis.breakpoint() was called. */
int luabp; /* Stop at next line because server.breakpoint() was called. */
sds *src; /* Lua script source code split by line. */
int lines; /* Number of lines in 'src'. */
int currentline; /* Current line number. */
Expand All @@ -114,7 +114,7 @@ struct ldbState {

/* Perform the SHA1 of the input string. We use this both for hashing script
* bodies in order to obtain the Lua function name, and in the implementation
* of redis.sha1().
* of server.sha1().
*
* 'digest' should point to a 41 bytes buffer: 40 for SHA1 converted into an
* hexadecimal number, plus 1 byte for null term. */
Expand All @@ -135,12 +135,12 @@ void sha1hex(char *digest, char *script, size_t len) {
digest[40] = '\0';
}

/* redis.breakpoint()
/* Adds server.breakpoint() function used by lua debugger.
*
* Allows to stop execution during a debugging session from within
* the Lua code implementation, like if a breakpoint was set in the code
* immediately after the function. */
int luaRedisBreakpointCommand(lua_State *lua) {
int luaServerBreakpointCommand(lua_State *lua) {
if (ldb.active) {
ldb.luabp = 1;
lua_pushboolean(lua,1);
Expand All @@ -150,12 +150,12 @@ int luaRedisBreakpointCommand(lua_State *lua) {
return 1;
}

/* redis.debug()
/* Adds server.debug() function used by lua debugger
*
* Log a string message into the output console.
* Can take multiple arguments that will be separated by commas.
* Nothing is returned to the caller. */
int luaRedisDebugCommand(lua_State *lua) {
int luaServerDebugCommand(lua_State *lua) {
if (!ldb.active) return 0;
int argc = lua_gettop(lua);
sds log = sdscatprintf(sdsempty(),"<debug> line %d: ", ldb.currentline);
Expand All @@ -167,14 +167,14 @@ int luaRedisDebugCommand(lua_State *lua) {
return 0;
}

/* redis.replicate_commands()
/* Adds server.replicate_commands()
*
* DEPRECATED: Now do nothing and always return true.
* Turn on single commands replication if the script never called
* a write command so far, and returns true. Otherwise if the script
* already started to write, returns false and stick to whole scripts
* replication, which is our default. */
int luaRedisReplicateCommandsCommand(lua_State *lua) {
int luaServerReplicateCommandsCommand(lua_State *lua) {
lua_pushboolean(lua,1);
return 1;
}
Expand Down Expand Up @@ -205,27 +205,27 @@ void scriptingInit(int setup) {
lctx.lua_scripts_lru_list = listCreate();
lctx.lua_scripts_mem = 0;

luaRegisterRedisAPI(lua);
luaRegisterServerAPI(lua);

/* 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. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

lua_getglobal(lua,"server");

/* redis.breakpoint */
/* server.breakpoint */
lua_pushstring(lua,"breakpoint");
lua_pushcfunction(lua,luaRedisBreakpointCommand);
lua_pushcfunction(lua,luaServerBreakpointCommand);
lua_settable(lua,-3);

/* redis.debug */
/* server.debug */
lua_pushstring(lua,"debug");
lua_pushcfunction(lua,luaRedisDebugCommand);
lua_pushcfunction(lua,luaServerDebugCommand);
lua_settable(lua,-3);

/* redis.replicate_commands */
/* server.replicate_commands */
lua_pushstring(lua, "replicate_commands");
lua_pushcfunction(lua, luaRedisReplicateCommandsCommand);
lua_pushcfunction(lua, luaServerReplicateCommandsCommand);
lua_settable(lua, -3);

lua_setglobal(lua,"redis");
lua_setglobal(lua,"server");

/* Add a helper function we use for pcall error reporting.
* Note that when the error is in the C function we want to report the
Expand All @@ -234,7 +234,7 @@ void scriptingInit(int setup) {
{
char *errh_func = "local dbg = debug\n"
"debug = nil\n"
"function __redis__err__handler(err)\n"
"function __server__err__handler(err)\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest aliasing __server__err__handler to __redis__err__handler to address Madelyn's compat concern.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in next commit.

" local i = dbg.getinfo(2,'nSl')\n"
" if i and i.what == 'C' then\n"
" i = dbg.getinfo(3,'nSl')\n"
Expand Down Expand Up @@ -583,7 +583,7 @@ void evalGenericCommand(client *c, int evalsha) {
evalCalcFunctionName(evalsha, c->argv[1]->ptr, funcname);

/* Push the pcall error handler function on the stack. */
lua_getglobal(lua, "__redis__err__handler");
lua_getglobal(lua, "__server__err__handler");

/* Try to lookup the Lua function */
lua_getfield(lua, LUA_REGISTRYINDEX, funcname);
Expand Down Expand Up @@ -1375,7 +1375,7 @@ char *ldbRedisProtocolToHuman_Double(sds *o, char *reply) {
/* Log a RESP reply as debugger output, in a human readable format.
* If the resulting string is longer than 'len' plus a few more chars
* used as prefix, it gets truncated. */
void ldbLogRedisReply(char *reply) {
void ldbLogServerReply(char *reply) {
sds log = sdsnew("<reply> ");
ldbRedisProtocolToHuman(&log,reply);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ldbLogWithMaxLen(log);
Expand Down Expand Up @@ -1516,30 +1516,30 @@ void ldbEval(lua_State *lua, sds *argv, int argc) {
}

/* Implement the debugger "server" command. We use a trick in order to make
* the implementation very simple: we just call the Lua redis.call() command
* the implementation very simple: we just call the Lua server.call() command
* implementation, with ldb.step enabled, so as a side effect the command
* and its reply are logged. */
void ldbRedis(lua_State *lua, sds *argv, int argc) {
void ldbServer(lua_State *lua, sds *argv, int argc) {
int j;

if (!lua_checkstack(lua, argc + 1)) {
/* Increase the Lua stack if needed to make sure there is enough room
* to push 'argc + 1' elements to the stack. On failure, return error.
* Notice that we need, in worst case, 'argc + 1' elements because we push all the arguments
* given by the user (without the first argument) and we also push the 'redis' global table and
* 'redis.call' function so:
* (1 (redis table)) + (1 (redis.call function)) + (argc - 1 (all arguments without the first)) = argc + 1*/
ldbLogRedisReply("max lua stack reached");
* given by the user (without the first argument) and we also push the 'server' global table and
* 'server.call' function so:
* (1 (server table)) + (1 (server.call function)) + (argc - 1 (all arguments without the first)) = argc + 1*/
ldbLogServerReply("max lua stack reached");
return;
}

lua_getglobal(lua,"redis");
lua_getglobal(lua,"server");
lua_pushstring(lua,"call");
lua_gettable(lua,-2); /* Stack: redis, redis.call */
lua_gettable(lua,-2); /* Stack: server, server.call */
for (j = 1; j < argc; j++)
lua_pushlstring(lua,argv[j],sdslen(argv[j]));
ldb.step = 1; /* Force redis.call() to log. */
lua_pcall(lua,argc-1,1,0); /* Stack: redis, result */
ldb.step = 1; /* Force server.call() to log. */
lua_pcall(lua,argc-1,1,0); /* Stack: server, result */
ldb.step = 0; /* Disable logging. */
lua_pop(lua,2); /* Discard the result and clean the stack. */
}
Expand Down
4 changes: 2 additions & 2 deletions src/function_lua.c
Original file line number Diff line number Diff line change
Expand Up @@ -423,11 +423,11 @@ int luaEngineInitEngine(void) {
luaEngineCtx *lua_engine_ctx = zmalloc(sizeof(*lua_engine_ctx));
lua_engine_ctx->lua = lua_open();

luaRegisterRedisAPI(lua_engine_ctx->lua);
luaRegisterServerAPI(lua_engine_ctx->lua);

/* Register the library commands table and fields and store it to registry */
lua_newtable(lua_engine_ctx->lua); /* load library globals */
lua_newtable(lua_engine_ctx->lua); /* load library `redis` table */
lua_newtable(lua_engine_ctx->lua); /* load library `server` table */

lua_pushstring(lua_engine_ctx->lua, "register_function");
lua_pushcfunction(lua_engine_ctx->lua, luaRegisterFunction);
Expand Down