[Fix issue #1521] wrong param sequence of lpushx&rpushx #1525
Conversation
AlexStocks
changed the title
|
@ForestLH 处理下文件冲突 |
src/storage/tests/lists_test.cc
Outdated
| @@ -711,7 +711,7 @@ TEST_F(ListsTest, LPushxTest) { | |||
| ASSERT_TRUE(elements_match(&db, "GP1_LPUSHX_KEY", {"o", "o", "o"})); | |||
|
|
|||
| // "x" -> "o" -> "o" -> "o" | |||
| s = db.LPushx("GP1_LPUSHX_KEY", "x", &num); | |||
| s = db.LPushx("GP1_LPUSHX_KEY", {"x"}, &num); | |||
There was a problem hiding this comment.
既然支持的是多个 elements，那下面每个测试用例都再补加上这么几个 case：
1 插入的 元素格式为空；
2 插入的元素个数是3；
There was a problem hiding this comment.
我加了部分，不知道够不够
尽可能的多加些能想到的 case
There was a problem hiding this comment.
既然支持的是多个 elements，那下面每个测试用例都再补加上这么几个 case： 1 插入的 元素格式为空； 2 插入的元素个数是 3；
不知道这俩测试 case 添加了吗？
There was a problem hiding this comment.
既然支持的是多个 elements，那下面每个测试用例都再补加上这么几个 case： 1 插入的 元素格式为空； 2 插入的元素个数是 3；
不知道这俩测试 case 添加了吗？
抱歉，我给忘了，我周末再加可以吗，周中有点儿忙
There was a problem hiding this comment.
既然支持的是多个 elements，那下面每个测试用例都再补加上这么几个 case： 1 插入的 元素格式为空； 2 插入的元素个数是 3；
不知道这俩测试 case 添加了吗？
抱歉，我给忘了，我周末再加可以吗，周中有点儿忙
当然可以呀，哈哈
Signed-off-by: Hao Lee <[email protected]>
Signed-off-by: Hao Lee <[email protected]>
Signed-off-by: Hao Lee <[email protected]>
![cr-gpt[bot]](https://avatars.githubusercontent.com/in/292659?s=60&v=4){kind=small}
| std::string value_; | ||
| void DoInitial() override; | ||
| std::vector<std::string> values_; | ||
| virtual void DoInitial() override; | ||
| }; | ||
| #endif |
There was a problem hiding this comment.
From the code patch provided, it seems that two commands, LPushxCmd and RPushxCmd, have been modified. The changes involve replacing a single value string argument with a vector of strings to store multiple values.
Assuming that the rest of the code implementation is correct, this change should reduce the possibility of bugs caused by incorrect usage of the command with multiple values. However, the implementation details of the DoInitial function are not provided, so any suggestions for improving performance or reducing risk related to this function cannot be made.
| std::make_unique<RPushxCmd>(kCmdNameRPushx, 3, kCmdFlagsWrite | kCmdFlagsSingleSlot | kCmdFlagsList); | ||
| std::unique_ptr<Cmd> rpushxptr = | ||
| std::make_unique<RPushxCmd>(kCmdNameRPushx, -3, kCmdFlagsWrite | kCmdFlagsSingleSlot | kCmdFlagsList); | ||
|
|
||
| cmd_table->insert(std::pair<std::string, std::unique_ptr<Cmd>>(kCmdNameRPushx, std::move(rpushxptr))); | ||
|
|
||
| // Zset |
There was a problem hiding this comment.
The code patch involves initializing a command table. The changes made are to modify the number of arguments required for two commands (LPushx and RPushx) from 3 to -3, allowing them to accept variable arguments.
The modification appears to be valid and makes sense if the commands are designed to accept variable arguments. However, it's difficult to comment on whether there are any bug risks or improvements that can be made without knowing more context about the overall system and its functionality.
A suggestion for improvement could be to ensure that there are appropriate checks in place to handle the variable number of arguments that the modified commands can now accept, to avoid potential issues related to memory allocation and usage.
| } | ||
| void RPushxCmd::Do(std::shared_ptr<Slot> slot) { | ||
| uint64_t llen = 0; | ||
| rocksdb::Status s = slot->db()->RPushx(key_, value_, &llen); | ||
| rocksdb::Status s = slot->db()->RPushx(key_, values_, &llen); | ||
| if (s.ok() || s.IsNotFound()) { | ||
| res_.AppendInteger(llen); | ||
| } else { |
There was a problem hiding this comment.
The code seems to be adding support for pushing multiple values to the Redis lists using the commands LPUSHX and RPUSHX. Here are some improvements and bug risks that can be considered:
- It's a good practice to include comments that describe what the functions do, e.g.,
DoInitial()andDo(). - The variable
poscould be improved to be more descriptive, likearg_index, so it would be easier to understand its purpose. - There is no check for whether there are any elements to push into the list or not. When
argv_.size() <= 2, the code still tries to access index 2 for values_ which will cause an out-of-bounds error. - It's unclear how
argv_[2]should be handled, since it is no longer assigned tovalue_. Should it be ignored, added tovalues_, or throw an error? - It's unclear whether all of the elements in
values_should be pushed onto the list together as a batch in a single database operation, or they should each result in their own individual database operation, effectively pushing them separately. This could have implications on performance and atomicity guarantees.
Regarding potential bugs:
- When
rocksdb::Status s = slot->db()->RPushx(key_, values_, &llen);is called, it is not clear whether this function handles empty or null lists correctly. - Any non-integer arguments input could produce strange errors down the line.
- If the
key_parameter is not of type list, a Redis exception should be thrown.
Overall, improving documentation, handling edge cases, and clarifying behavior in corner cases would improve the overall robustness and reliability of this codebase.
|
在等两三天吧，如果确实没啥时间添加 ut，我就 merge 了 |
fix #1521
解决lpushx和rpushx多元素插入的问题