Feature/txn

[ForestLH]

Fixes: #1446
验证一下代码思路，留了一些注释方便看，还有一些测试功能未做完
请老师们帮忙看一下实现思路有没有问题，感谢

1. 在PikaClientConn::DoCmd中负责检测是否是事务相关的命令，比如如果是multi命令，那么就将PikaClientConn状态设置成为开启事务，以后再发送给此连接的命令（非exec，multi，watch，discard）则不执行，而是直接放入PikaClientConn的事务队列中去。
2. 关于乐观锁，在dispatcher里面维护了所有conn对象所watch的key，一旦这个key被其他conn对象所修改，那么就会将watch这个key的所有conn对象的事务状态设置成为WatchFailed（在PikaClientConn::DoCmd这个函数里面会有检测，如果A连接执行一个命令，这个命令是write，就会去拿到这个命令操作的key，然后根据这个key，交给dispatcher，让dispatcher设置所有watch这个key的事务连接，所以这个每个conn对象的txn_state_字段需要加锁保护）。
3. 关于binlog部分，主机只有在执行exec的时候，才会涉及到binlog，具体来说是，在ExecCmd的Do方法中，去conn对象中拿到当前连接事务队列里面的所有命令，然后加锁，依次执行，并且会写入binlog，但是Multi，Exec此类命令不写binlog，所以对于从机来说，是无感知事务的。
4. 从机消费binlog的时候，本来是可以做成，读到watch命令，就将接下来的（直到exec命令出现）命令都加到内存，而不执行，读到exec命令之后，再执行这些命令。但是这个实现方式的话，对于从机以全量更新的方式去同步binlog来说是很难实现的，而且考虑到，这些事务里面的命令主机既然已经写binlog了，那么就都是有意义的。

7月16日更新

1. 其他没有变化
2. 上一版本忘记了处理在事务中遇上flushall，或者是flushdb此类命令，这一版本有解决
3. 主要变化就是为了减小加锁的粒度，目前的话，加锁思路也是在，执行事务之前将会遍历所有队列中的命令，会将涉及到的所有目标给加锁

• 如果队列中包含flushall命令，会在整个事务的执行期间，锁住所有的db，和没有事务时，加锁方式保持一致
• 如果队列中包含flushdb命令，会在将这个db给加锁，直到事务执行结束，才会释放
• 如果队列中是普通的命令，那么就会涉及到的slot给加上读锁，然后再这个命令涉及到的key给加上写锁，不论是读还是写，都必须给涉及到的key加上写锁

具体更改：

1. 修改了select命令与redis不一致的情况，redis在事务中出现了select命令的话，在入队的时候不会检测，只有在exec的时候才会去执行select命令，但是pika却会在select命令的SelectCmd::DoInitial()函数会执行后面切换db的参数检测，所以，将其移到Do函数中去
2. 重构了在Cmd抽象类的Execute方法，这个Cmd::Execute()方法在基类中的实现我觉得是有点儿冗余的，直接在具体的命令中的Execute函数中实现就好了
3. 将flushall和flushdb这两个命令的加锁方法给修改了一下，做出了一个不加锁的DoWithoutLock函数，方便在事务中去调用
4. ExecCmd::SetCmdsVec()函数用于在ExecCmd::Lock()和ExecCmd::Do()执行前，给构造出一个带有各种信息的cmd的队列，因为cmd是不带比如slot或者是db这类信息的，但是在Do函数中没法拿到，因为去get这些信息的函数里面都有锁，可是在执行Do()函数时候已经加好锁了，所以必须提前准备好。
5. ExecCmd类中增加几个hash set或者是hash map，为了记录加锁的资源

• lock_db_:涉及到的db就会加锁，只有flushall和flushdb会往里面加
• lock_slot_keys_:涉及到的keys，普通命令会往里面加
• r_lock_slots_:涉及到的slot，以加读锁的方式去加锁
• is_lock_rm_slots_:是否锁了PikaReplicaManager::slots_rw_，就是flushall和flushdb会锁

[cr-gpt]

Overall, the code patch is simple and straightforward. It appears that the only change being made is to the value of the "databases" parameter, which is being updated from 1 to 8.

There don't seem to be any immediate bug risks associated with this change, but it's possible that it could have unintended consequences if other parts of the system are relying on the previous value of the parameter.

As for improvement suggestions, perhaps adding a comment explaining the rationale behind the change would be useful for future readers of the code. Additionally, some documentation around the configuration file format (if it doesn't already exist) could help prevent future mistakes or confusion when making changes to these types of parameters.

[cr-gpt]

The code patch seems to add Redis transactions support for a PikaClientConn class. Here are some suggestions and improvements:

• It seems that the destructor is overridden but not doing anything. It could be removed altogether as it does not seem to provide any additional functionality.
• The TxnStateBitMask class containing static constants is related to transaction states, but the use of bit shifting is unclear. Enum values could be used instead.
• There are utility methods for managing transactions (e.g., IsInTxn(), AddKeysToWatch()) in the PikaClientConn class, but there is no documentation explaining how they should be used or what their side effects might be. Adding docstrings would help improve code readability and maintainability.
• The comment about the large txn_db_mu_ lock mentions that "there will be an issue with the order in which locks are acquired." This statement is not explained further, so users of the class may not understand the rationale behind the design decision or the potential for deadlock if the locking scheme is not followed properly.
• It's difficult to determine whether there are any bugs without more context or code coverage information.

[cr-gpt]

The code patch appears to add a set of new commands and related functionality for transactions. Here are a few observations and suggestions:

• It's good that the required headers have been included, and the namespace usage is consistent.
• Adding new commands and related data structures should be reviewed thoroughly to ensure they don't affect existing functionality by introducing side effects or bugs.
• The CmdRes class has been modified to include response values for the new transaction-related commands. However, it might need further testing to ensure it works correctly with every transaction scenario.
• The ProcessExecCmd virtual function has been added to allow processing of EXEC command execution. The implementation would also require proper testing to catch any side effects or bugs in the existing application flow.
• There seems to be a typo in the spelling of "invalid" in kInvaildTranscation, and it may go unnoticed during the review process. A spell-check can avoid such issues.
• It's unclear why the cmd_names are initialized to string constants; concern should be given to their lifespan and scope.
• Some comments that explain the purpose of each data structure would help other developers understand it more easily.
• There could be additional methods or properties on the Cmd object like ID, Priority or Token, which could be useful for scheduling purposes.
• Lastly, running meaningful unit tests to validate all functionalities would be highly recommended before merging the patch.

[cr-gpt]

The code review consists of the following points:

• The code adds a new class TxnStateBitMask to manage transaction states, which seems like a reasonable and clean approach.
• The default destructor is modified to be non-default in PikaClientConn, but no reason is given for this change, so it's hard to tell if it was necessary or not.
• The use of std::queue<std::shared_ptr<Cmd>> for storing transaction commands seems appropriate for managing transactions.
• The use of locks for txn_state_ and txn_db_mu_ also appears appropriate since these variables are shared between threads.
• There are some incomplete comments, such as //TODO(leeHao): 将一个超时关闭的连接所watch的key给从全局的那个变量里面给清除掉 that need to be addressed.

Some possible improvements include providing more detailed comments, catching potential exceptional scenarios, adding unit tests, and implementing standard C++ exception handling to improve code robustness.

[cr-gpt]

The provided code is a set of header and class declarations related to transactions in Pika, which includes multi, exec, discard, watch, and unwatch commands.

Without the definitions of these classes' member functions, it is difficult to identify any potential bugs or improvements. However, there seems to be no syntax or formatting issues in the provided code patch.

One potential improvement suggestion would be to add comments explaining each class's purpose and usage in more detail. It helps other programmers understand and use these classes in the future.

[cr-gpt]

Overall, the code does not seem to contain any syntax or semantic errors. However, some suggestions for improvement are:

1. The destructor of PikaClientConn is overriden but doesn't have any implementation. Since the parent class has a virtual destructor, it should be explicitly called in the child class destructor or its only declaration can be left as default.

2. The use of raw locks and mutexes might lead to deadlocks if proper care is not taken. Consider using RAII wrappers like std::lock_guard or std::unique_lock to mitigate this risk.

3. Some comments mention "db" but it's unclear what it refers to. Adding more descriptive comments and naming conventions would improve its readability and maintainability.

4. The current design uses a queue and a bitset to implement a transaction. This may suffice, but there are more efficient and flexible ways to structure transactions, such as using a directed acyclic graph(DAG) to represent dependencies among operations within a transaction.

5. The name PikaClientConn could be improved by following existing conventions in the project or industry standards.

[cr-gpt]

The code patch adds functionality to watch keys and get involved transactions in a multithreaded environment. Here are some suggestions:

1. It is good that the code is making use of mutexes to protect concurrent access to shared data structures. However, it's not clear whether these mutexes are defined and initialized correctly. Make sure they are instantiated as appropriate types (e.g., std::mutex, std::shared_mutex) and properly constructed with their default constructors.

2. In AddWatchKeys, there is a mistake in the emplace call to key_conns_map_: it should be an argument containing the key and the value (i.e., key, std::set<std::shared_ptr<NetConn>>()). The same applies to key_conns_map_.erase(key).

3. In RemoveWatchKeys, we could avoid the double map lookup by iterating directly over the set of keys for a given client_connection instead of looking them up in conn_keys_map_. e.g:

  auto it = key_conns_map_.begin();
  while (it != key_conns_map_.end()) {
    it->second.erase(client_conn);
    if (it->second.empty()) it = key_conns_map_.erase(it);
    else ++it;
  }

4. In GetInvolvedTxn, since the function returns a vector of shared pointers, we can use a range-based for loop to simplify the code a bit, e.g:

  for (const auto& [key, conns] : key_conns_map_) {
    if (std::find(keys.begin(), keys.end(), key) == keys.end()) continue;
    for (const auto& conn : conns) involved_conns.emplace_back(conn);
  }

5. Overall, the code looks good, but it would be helpful to add some comments that explain the purpose of each function and clarify any ambiguities in the code.

[wanghenshui]

单独设计事务的binlogOp，比如BEGAIN_TXN END_TXN，回滚binlog的时候要检查 是不是配对的

[wanghenshui]

同步binlog的时候，在BEGAN_TXN END_TXN中间的op不执行，直到配对检查到之后再执行

[ForestLH]

是这样的水哥，我这两天怎么想都觉得在从机消费binlog的时候，来检查是不是配对的这个很难做。

我理解全量更新的话，从机那边是不用做读取和解析log并执行log中命令的操作的，直接将log文件替换即可，所以其实是不知道这个log里面包不包含事务的，所以我觉得目前全量更新很难做成下面这样子，即在读到事务开始便将主机发的binlog给加入内存，而不写磁盘，等到读到exec命令，再统一写磁盘，然后增量更新的话，是可以做的。

那么能否这样？

1. 在主节点，执行到事务这块儿的时候，只有执行到exec命令，且事务当中命令有效的情况下，就加锁，开始执行本次事务中各个命令的Do，以及DoBinlog（不包含multi和exec，watch，unwatch，这几个命令不写binlog），写完之后返回给客户端成功。
2. 在从机消费主机的binlog的时候，因为都是有效的，其实不需要回退这个功能。

[wanghenshui]

你的意思是从机只同步事务的结果。也可以

[wanghenshui]

目前的这种设计，跨db的事务如何处理？

[ForestLH]

目前的处理是在Exec的Do函数中，先拿取一把大锁g_pika_server->dbs_rw_，再拿取涉及到的所有Slot的锁，然后再处理所有的命令，处理完成之后，统一释放所有锁

[wanghenshui]

你这种写法会不会影响别的命令。事务的锁应该只锁key

[ForestLH]

这样写是一定会影响其他命令的，只要是涉及到这个slot的命令都会被卡住。
好像只锁key也确实可以。

[cr-gpt]

Here are some suggestions and improvements for the code:

1. In the constructor PikaClientConn, initialize is_pubsub_ explicitly along with other member variables to improve readability.

2. In the destructor ~PikaClientConn, use curly braces {} instead of default. Even though the default destructor is fine, using curly braces makes it explicit.

3. Consider making the member function GetCurrentDb a const member function since it only retrieves the value and doesn't modify the object.

4. Provide descriptive comments for the PushCmdToQue function and other functions in the Txn section to explain their purpose and usage.

5. Consider using more descriptive variable names rather than abbreviations. For example, replace db_keys with databaseKeys or something similar. This will improve code understandability.

6. Review the error handling and exception handling mechanisms in the code to ensure that all potential error scenarios are properly handled.

7. Consider adding unit tests for the code, especially for the functions related to transaction processing, to ensure their correctness.

8. Review the usage of locks and mutexes in the code to prevent any potential race conditions when accessing shared data structures, such as txn_cmd_que_ and txn_state_. Ensure that the locking mechanism is correctly implemented to avoid deadlocks.

These suggestions aim to improve the overall clarity, maintainability, and reliability of the code. It's important to thoroughly test and validate the code after implementing any changes to ensure its correctness.

[wanghenshui]

尽量分片锁，然后锁一组key，避免影响别的命令

…

------------------ 原始邮件 ------------------ 发件人: ***@***.***>; 发送时间: 2023年7月12日(星期三) 下午2:30 收件人: ***@***.***>; 抄送: ***@***.***>; ***@***.***>; 主题: Re: [OpenAtomFoundation/pika] Feature/txn (PR #1585) @ForestLH commented on this pull request. In src/pika_command.cc: > @@ -547,6 +568,38 @@ void Cmd::ProcessFlushAllCmd() { } res_.SetRes(CmdRes::kOk); } +/** + * TODO(leeHao): 这个exec命令本身的binlog我不确定如何处理,我感觉应该还是需要写的，因为如果执行队列中的命令，执行到了一半但是退出了 + * 那么下次启动的时候，应该回滚部分操作？ + * TODO(leeHao): 还有个得到执行时间的函数,这个统计命令时间，是统计事务队列中，每个命令单独统计还是统计一整个队列的执行时长 + * 这里因为是多个db的锁，所以需要注意执行顺序，否则就会死锁，我的选择是，先拿取一把大锁 + * 然后将所有的锁给拿齐了，再释放掉这把大锁 + */ 这样写是一定会影响其他命令的，只要是涉及到这个slot的命令都会被卡住。 好像只锁key也确实可以。 — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: ***@***.***>

[AlexStocks]

尽量分片锁，然后锁一组key，避免影响别的命令
…
------------------ 原始邮件 ------------------ 发件人: @.>; 发送时间: 2023年7月12日(星期三) 下午2:30 收件人: @.>; 抄送: @.>; @.>; 主题: Re: [OpenAtomFoundation/pika] Feature/txn (PR #1585) @ForestLH commented on this pull request. In src/pika_command.cc: > @@ -547,6 +568,38 @@ void Cmd::ProcessFlushAllCmd() { } res_.SetRes(CmdRes::kOk); } +/** + * TODO(leeHao): 这个exec命令本身的binlog我不确定如何处理,我感觉应该还是需要写的，因为如果执行队列中的命令，执行到了一半但是退出了 + * 那么下次启动的时候，应该回滚部分操作？ + * TODO(leeHao): 还有个得到执行时间的函数,这个统计命令时间，是统计事务队列中，每个命令单独统计还是统计一整个队列的执行时长 + * 这里因为是多个db的锁，所以需要注意执行顺序，否则就会死锁，我的选择是，先拿取一把大锁 + * 然后将所有的锁给拿齐了，再释放掉这把大锁 + / 这样写是一定会影响其他命令的，只要是涉及到这个slot的命令都会被卡住。 好像只锁key也确实可以。 — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.**>

水哥这个建议很好，使用分片锁吧。Pika 分片锁的代码是从 RocksDB 抄过来的，效率还是很在线的。

[cr-gpt]

Based on the provided code patch, it appears that you have added several friend classes to the PikaReplicaManager class. This grants those classes access to the private and protected members of PikaReplicaManager. This change seems reasonable assuming that you have a valid reason for doing so.

Without more context or the complete codebase, it is challenging to provide an extensive code review. However, based on the provided information, here are a few general suggestions:

1. Documentation: Ensure that the purpose and behavior of the PikaReplicaManager class and its associated functions (Start(), Stop(), etc.) are adequately documented. Clear documentation will help improve code maintainability.
2. Error Handling: Review the error handling mechanism in place. It's essential to handle potential errors gracefully and provide appropriate feedback or recovery mechanisms where needed.
3. Testing: It's good practice to have unit tests covering the functionality of PikaReplicaManager and any associated classes. Automated tests can help catch bugs and ensure that changes made to the codebase do not introduce new issues.
4. Code Organization: Check if there is an opportunity to refactor the code for better organization and readability. Aim for clear and concise functions that follow established coding standards and best practices.

Remember, a thorough code review often requires a comprehensive understanding of the entire codebase. Consider involving other developers familiar with the project to perform a more detailed and contextualized review.

[cr-gpt]

Based on the code patch you provided, here are a few observations:

1. In line 38, there seems to be a redundant inclusion of "pika_migrate_thread.h" since it's already included in line 11.

2. In line 39, there is an additional inclusion of "pika_transaction.h". Make sure it is necessary and doesn't conflict with existing code.

3. It seems like classes named "FlushallCmd" and "ExecCmd" are being declared. Double-check if they are properly defined elsewhere.

4. Since this code snippet only provides the inclusion part and not the definition or implementation, it's challenging to identify specific bugs or improvements. A more thorough review would require examining the logic and functionality of the classes and functions involved.

Please provide more details or specific sections of the code if you would like a more comprehensive review.

[chejinge]

改回去

[chejinge]

这里改动的原因是啥，没看懂

[ForestLH]

这里是因为在PikaClientConn::TryWriteResp函数中，会将conn的resp数组给清空，由于command不持有这个string，所以这个string会被释放掉，但是如果是事务之中的命令的话，这个command会被加入到队列中，后续要使用，但是后续使用的时候就拿不到string了，于是我就变成了shared指针。

[ForestLH]

e1da3da

在此提交之中，还原了cmd中的 weak ptr