support lua script in pika #1724

A2ureStone · 2023-07-14T06:47:39Z

#1445
目前跑通了执行脚本的过程，还有细节需要添加。
EVAL命令原理：
将接受到的脚本注册成一个lua里的新函数，拿db写锁住整个pika的读写流程保证原子性，调用新注册的函数运行用户的脚本片段，函数返回结果会转成字符串传回客户端。
lua环境里有预先注册的redis.call函数，脚本运行redis.call的时候相当于调用LuaRedisGenericCommand函数，从lua里获取参数执行DoCmd，由于这个时候可能还有在拿db读锁，所以加了个检查避免重新拿锁。命令相关逻辑执行完后，将返回结果转成lua类型返回。

Fixes: #1445

CMakeLists.txt

include/pika_client_conn.h

.gitignore

src/script/lua-cmsgpack/.gitignore

levy5307 · 2023-07-14T07:43:24Z

include/pika_script.h

@@ -0,0 +1,43 @@
+#ifndef PIKA_SCRIPT_H


#pragma once

目前pika头文件都是用宏的方式，不过采取两种都使用应该是保险方案？可以参考这个回答 https://www.zhihu.com/question/40990594

include/pika_client_conn.h

cr-gpt · 2023-07-15T08:42:48Z

include/pika_command.h

@@ -454,6 +471,9 @@ class Cmd : public std::enable_shared_from_this<Cmd> {
  bool is_admin_require() const;
  bool is_single_slot() const;
  bool is_multi_slot() const;
+  bool is_script() const;
+  bool is_sort_for_script() const;
+  bool is_random() const;
  bool HashtagIsConsistent(const std::string& lhs, const std::string& rhs) const;
  uint64_t GetDoDuration() const { return do_duration_; };



Based on the provided code patch, here are some observations regarding bug risks and improvement suggestions:

In the CmdFlagsMask enumeration, the flag values assigned to kCmdFlagsMaskRandom, kCmdFlagsMaskSortForScript, and kCmdFlagsMaskScript are incorrect. They should be 8192, 16384, and 32768 respectively, instead of 4096, 8192, and 16384.

In the CmdFlags enumeration, the flags KCmdFlagsNoRandom, KCmdFlagsNoSortForScript, and KCmdFlagsScript should have a value of 8192, 16384, and 0 respectively, instead of being assigned values within the range of other flags.

The is_script(), is_sort_for_script(), and is_random() methods are missing their implementations. They can be implemented by checking the command flags using bitwise operations.

The CmdRes enum does not seem to be used in the provided code patch. If it is used elsewhere, ensure that all the necessary error cases are handled properly.

Apart from these specific points, a comprehensive code review would require examining the entire project or context in which this code patch is being applied. It's important to review the rest of the codebase for any potential issues related to error handling, memory management, concurrency, security, and overall design.

include/pika_client_conn.h

include/pika_script.h

A2ureStone · 2023-07-15T17:43:17Z

src/pika_client_conn.cc

@@ -30,6 +31,106 @@ PikaClientConn::PikaClientConn(int fd, const std::string& ip_port, net::Thread*
  auth_stat_.Init();
 }

+// 主从redis.call的命令都从这里执行， 特别注意从的eval命令会调用redis.call
+std::shared_ptr<Cmd> PikaClientConn::DoCmdInLua(const PikaCmdArgsType& argv, const std::string& opt,


DoCmdLua是DoCmd的改造，主节点和从节点的redis.call都是调用这个函数进行参数检查的，我去掉了一些在lua里不需要的检查比如说auth，pubsub这些，各位大佬看看有没有不合理的地方

A2ureStone · 2023-07-15T17:49:32Z

目前的主从方案是redis.call利用了c_ptr->Execute();这里会对写命令同步，因此每个lua里调用的写命令都会被同步。redis里的方案是同步执行的lua代码，这样能够减少发送次数。

cr-gpt · 2023-07-17T08:10:24Z

include/pika_command.h

@@ -454,6 +473,9 @@ class Cmd : public std::enable_shared_from_this<Cmd> {
  bool is_admin_require() const;
  bool is_single_slot() const;
  bool is_multi_slot() const;
+  bool is_script() const;
+  bool is_sort_for_script() const;
+  bool is_random() const;
  bool HashtagIsConsistent(const std::string& lhs, const std::string& rhs) const;
  uint64_t GetDoDuration() const { return do_duration_; };



The code patch includes the following changes:

Added constants for Lua script commands, such as eval, evalsha, and script.

Added new command flags related to scripts and sorting (kCmdFlagsMaskSortForScript, kCmdFlagsMaskScript, KCmdFlagsNoSortForScript, KCmdFlagsSortForScript).

Added new error codes for invalid key numbers and missing script (kInvalidKeyNums, kNoScript).

Modified the CmdRes class to handle the new error codes.

Added member functions to the Cmd class to check if a command is a script, sort for script, or random.

Without seeing the complete context and usage of these changes, it's challenging to provide specific bug risks or improvement suggestions. However, here are some general recommendations:

Review the usage of the new Lua script-related commands and ensure they are correctly implemented and utilized in your codebase.

Verify that the new command flags (kCmdFlagsMaskSortForScript, kCmdFlagsMaskScript, etc.) are used consistently throughout the codebase to ensure accurate command classification and handling.

Test the error handling logic in the CmdRes class to ensure it behaves as expected for the new error codes introduced.

Consider adding comments or documentation to explain the purpose and usage of the new constants, flags, and error codes to improve code understandability.

Conduct thorough testing to ensure the correctness and stability of the modified code.

Remember, a thorough code review requires a comprehensive understanding of the entire codebase, its requirements, and its objectives. Therefore, consider seeking feedback from domain experts or experienced developers who are familiar with the project to obtain a more nuanced and informed review.

A2ureStone · 2023-07-18T08:43:02Z

src/pika_server.cc

+  std::shared_lock l(dbs_rw_, std::defer_lock);
+  // TODO improve and find other deadlock
+  if (!lua_calling_) {
+    l.lock();


在执行EVAL的时候会拿dbs_rw_写锁，然后如果redis.call执行某个命令调用的函数也需要拿这个锁的话，就会死锁，加了这个检查，各位可以看看哪些命令需要进入的函数还可能拿锁

A2ureStone · 2023-07-18T08:45:18Z

目前实现的功能已经跑过单测，具体在tests/unit/scripting.tcl

conf/pika.conf

include/pika_conf.h

cr-gpt · 2023-07-21T18:40:45Z

conf/pika.conf

+# Set it to 0 or a negative value for unlimited execution without warnings.
+lua-time-limit : 5000
+
+
 # The [password of administrator], which is empty by default.
 # [NOTICE] If this admin password is the same as user password (including both being empty),
 # the value of userpass will be ignored and all users are considered as administrators,


The code patch you provided appears to be modifying a configuration file for Redis. Here is a brief review of the changes made:

A new configuration parameter lua-time-limit has been added with a value of 5000. This parameter sets the maximum execution time (in milliseconds) for Lua scripts in Redis.

Improvement suggestions:

It is generally a good practice to include comments that explain the purpose and usage of each configuration parameter. You may consider adding comments describing the functionality of lua-time-limit and its recommended values.

It's important to carefully choose the value for lua-time-limit based on your specific requirements and workload. Consider evaluating the average execution time of your Lua scripts and setting a value that allows sufficient time for them to complete without causing timeouts.

Bug risks:
Without additional context or access to the entire codebase, it is difficult to identify other bugs or issues. A code review typically involves examining the code logic, error handling, security considerations, and performance optimizations. If you have specific concerns or need assistance with a particular section of code, please provide more information.

cr-gpt · 2023-07-21T19:12:27Z

include/pika_client_conn.h

@@ -73,6 +74,8 @@ class PikaClientConn : public net::RedisConn {

  std::shared_ptr<Cmd> DoCmd(const PikaCmdArgsType& argv, const std::string& opt,
                             const std::shared_ptr<std::string>& resp_ptr);
+  std::shared_ptr<Cmd> DoCmdInLua(const PikaCmdArgsType& argv, const std::string& opt,
+                             const std::shared_ptr<std::string>& resp_ptr); 

  void ProcessSlowlog(const PikaCmdArgsType& argv, uint64_t start_us, uint64_t do_duration);
  void ProcessMonitor(const PikaCmdArgsType& argv);


Based on the code patch you provided, here are some observations and suggestions:

In the PikaClientConn class:

The DealMessage function is currently not implemented (return 0;). Make sure this function is implemented correctly to handle Redis commands.

The DoBackgroundTask and DoExecTask functions are declared as static but not defined in the provided code. Make sure they are defined appropriately.

The ExecRedisCmdInLua function is declared but not implemented in the provided code. Implement this function if it's intended to be used.

Consider marking the IsPubSub() function as a const member function since it doesn't modify the object's state.

The GetCurrentTable() function is modified to be a const member function. This change allows calling the function on const objects of PikaClientConn.

It seems that the SetWriteCompleteCallback function is missing in the provided code.

The DoCmd and DoCmdInLua functions are introduced but not implemented in the provided code. Implement these functions based on your requirements.

Additional context or code may be necessary to provide a more thorough code review and identify potential bugs or improvements effectively.

Please make sure to complete the missing implementations and consider providing more details if there are other specific concerns or issues in your code.

skeleton for lua script

6ae9f3d

cr-gpt bot reviewed Jul 14, 2023

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

cr-gpt bot reviewed Jul 14, 2023

View reviewed changes

include/pika_client_conn.h Show resolved Hide resolved

remove submodule in 3rd lua library

41e2f86

cr-gpt bot reviewed Jul 14, 2023

View reviewed changes

.gitignore Show resolved Hide resolved

cr-gpt bot reviewed Jul 14, 2023

View reviewed changes

src/script/lua-cmsgpack/.gitignore Outdated Show resolved Hide resolved

levy5307 reviewed Jul 14, 2023

View reviewed changes

include/pika_client_conn.h Outdated Show resolved Hide resolved

script command implement

8f6609e

cr-gpt bot reviewed Jul 15, 2023

View reviewed changes

include/pika_client_conn.h Show resolved Hide resolved

cr-gpt bot reviewed Jul 15, 2023

View reviewed changes

sync every write command in script

d9099e2

cr-gpt bot reviewed Jul 15, 2023

View reviewed changes

include/pika_client_conn.h Show resolved Hide resolved

cr-gpt bot reviewed Jul 15, 2023

View reviewed changes

include/pika_script.h Show resolved Hide resolved

A2ureStone commented Jul 15, 2023

View reviewed changes

add base test to scripting

9b80cd9

cr-gpt bot reviewed Jul 17, 2023

View reviewed changes

A2ureStone commented Jul 18, 2023

View reviewed changes

J1senn added 2 commits July 20, 2023 02:02

response error when lua script timeout

94eed72

sync kill script, add timeout configure

cf70a0c

cr-gpt bot reviewed Jul 21, 2023

View reviewed changes

conf/pika.conf Outdated Show resolved Hide resolved

cr-gpt bot reviewed Jul 21, 2023

View reviewed changes

include/pika_conf.h Show resolved Hide resolved

revert databases default nums

d804b2a

cr-gpt bot reviewed Jul 21, 2023

View reviewed changes

J1senn added 2 commits July 22, 2023 02:45

Merge branch 'unstable' into lua

b4dab44

fix compile error after merge

33e26c1

cr-gpt bot reviewed Jul 21, 2023

View reviewed changes

yaoyinnan added the enhancement label Jul 27, 2023

J1senn added 3 commits September 2, 2023 20:02

Merge branch 'unstable' into lua

cc4f202

luaJIT as lua runtime

edec428

add replication test

9dfa437

chejinge changed the base branch from unstable to OpenAtom-Lua September 22, 2023 09:27

J1senn added 11 commits September 22, 2023 20:45

add get method for thread_rep

aaf023b

install libreadline in workflow

6708d3c

trigger workflow

5e6f8ba

pass -y to install

b6c7a51

use lua5.1.5 to cross platform

5ca3fe8

remove explicit lua folder

5bb14b0

remove explicit sol2

1fde439

remove not usred lua module file

8a305af

add libreadline in docker config

f52a583

use include/sol instead of single file

438916f

revert workflow

25179e1

yaoyinnan approved these changes Oct 20, 2023

View reviewed changes

yaoyinnan merged commit cbf068d into OpenAtomFoundation:OpenAtom-Lua Oct 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support lua script in pika #1724

support lua script in pika #1724

A2ureStone commented Jul 14, 2023 •

edited by yaoyinnan

Loading

levy5307 Jul 14, 2023

A2ureStone Oct 19, 2023

cr-gpt bot Jul 15, 2023

A2ureStone Jul 15, 2023

A2ureStone commented Jul 15, 2023

cr-gpt bot Jul 17, 2023

A2ureStone Jul 18, 2023

A2ureStone commented Jul 18, 2023

cr-gpt bot Jul 21, 2023

cr-gpt bot Jul 21, 2023

support lua script in pika #1724

support lua script in pika #1724

Conversation

A2ureStone commented Jul 14, 2023 • edited by yaoyinnan Loading

levy5307 Jul 14, 2023

Choose a reason for hiding this comment

A2ureStone Oct 19, 2023

Choose a reason for hiding this comment

cr-gpt bot Jul 15, 2023

Choose a reason for hiding this comment

A2ureStone Jul 15, 2023

Choose a reason for hiding this comment

A2ureStone commented Jul 15, 2023

cr-gpt bot Jul 17, 2023

Choose a reason for hiding this comment

A2ureStone Jul 18, 2023

Choose a reason for hiding this comment

A2ureStone commented Jul 18, 2023

cr-gpt bot Jul 21, 2023

Choose a reason for hiding this comment

cr-gpt bot Jul 21, 2023

Choose a reason for hiding this comment

A2ureStone commented Jul 14, 2023 •

edited by yaoyinnan

Loading