feat: supported 'ptype' command #1586

lqxhub · 2023-06-04T11:03:23Z

fix: #1541
支持 'ptype' 命令查询所有指定name 所有数据类型; 并且格式化了文件的其他代码.

现在 'del' 命令会删除name下所有类型的数据, 目前没有找到删除指定类型 name 的命令, 如果没有的话,应该也要支持吧

cr-gpt · 2023-06-04T11:03:36Z

include/pika_command.h

@@ -80,6 +80,7 @@ const std::string kCmdNameTtl = "ttl";
 const std::string kCmdNamePttl = "pttl";
 const std::string kCmdNamePersist = "persist";
 const std::string kCmdNameType = "type";
+const std::string kCmdNamePType = "ptype";
 const std::string kCmdNameScan = "scan";
 const std::string kCmdNameScanx = "scanx";
 const std::string kCmdNamePKSetexAt = "pksetexat";


It's difficult to fully review the code patch without the context of the entire source code. However, based on this snippet:

A new command kCmdNamePType has been added.

The existing commands kCmdNameTtl, kCmdNamePttl, kCmdNamePersist, kCmdNameType, kCmdNameScan, kCmdNameScanx, and kCmdNamePKSetexAt are still present.

There don't appear to be any obvious syntax errors or bugs introduced in this patch.

Here are some general suggestions for code review:

Ensure that the names of the new and existing commands are consistent with the conventions used elsewhere in the codebase.

Check that the implementation logic for each command is correct and efficient.

Consider writing and executing unit tests to confirm that all of the commands are working as expected.

Ensure that the new code is properly documented and adheres to any relevant coding standards or best practices.

cr-gpt · 2023-06-04T11:03:40Z

include/pika_kv.h

+ std::string key_;
+ void DoInitial() override;
+};
+
 class ScanCmd : public Cmd {
 public:
 ScanCmd(const std::string& name, int arity, uint16_t flag) : Cmd(name, arity, flag), pattern_("*") {}


The code patch seems to add a new class PTypeCmd which inherits from Cmd and overrides some of its virtual methods. The implementation looks fine with no apparent bug risk.

However, it would be helpful to have some more context on the purpose of this new class and how it fits into the existing codebase. Additionally, it's worth noting that brief code reviews are often insufficient for thorough evaluation of code quality, maintainability, and scalability.

cr-gpt · 2023-06-04T11:03:45Z

src/pika_command.cc

@@ -176,6 +176,9 @@ void InitCmdTable(CmdTable* cmd_table) {
 ////TypeCmd
 std::unique_ptr<Cmd> typeptr = std::make_unique<TypeCmd>(kCmdNameType, 2, kCmdFlagsRead | kCmdFlagsSingleSlot | kCmdFlagsKv);
 cmd_table->insert(std::pair<std::string, std::unique_ptr<Cmd>>(kCmdNameType, std::move(typeptr)));
+ ////PTypeCmd
+ std::unique_ptr<Cmd> pTypeptr = std::make_unique<PTypeCmd>(kCmdNamePType, 2, kCmdFlagsRead | kCmdFlagsSingleSlot | kCmdFlagsKv);
+ cmd_table->insert(std::pair<std::string, std::unique_ptr<Cmd>>(kCmdNamePType, std::move(pTypeptr)));
 ////ScanCmd
 std::unique_ptr<Cmd> scanptr = std::make_unique<ScanCmd>(kCmdNameScan, -2, kCmdFlagsRead | kCmdFlagsMultiSlot | kCmdFlagsKv);
 cmd_table->insert(std::pair<std::string, std::unique_ptr<Cmd>>(kCmdNameScan, std::move(scanptr)));


There don't seem to be any syntax errors in this patch, but without a broader context of the codebase, it's difficult to assess whether this change introduces any new bugs or risk.

As for possible improvements, the code shows the use of std::unique_ptr and std::pair which is consistent with modern C++ best practices. However, since C++11 introduced emplace, it might be cleaner to use it instead of creating a temporary object and then moving it into the map. For example:

cmd_table->emplace(kCmdNameType, std::make_unique<TypeCmd>(kCmdNameType, 2, kCmdFlagsRead | kCmdFlagsSingleSlot | kCmdFlagsKv));

这里按gpt的要求使用emplace吧

这个也是和之前的保持一致, 也是觉得统一改掉好点

lqxhub · 2023-06-04T11:19:02Z

简单测试

infdahai · 2023-06-07T12:07:48Z

src/storage/include/storage/storage.h

@@ -971,6 +970,9 @@ class Storage {
 // Reutrns the data type of the key
 Status Type(const std::string& key, std::string* type);

+ // Reutrns the data all type of the key
+ Status Type(const std::string& key, std::vector<std::string>& types);


函数名可能需要更具体点，与上面Type()区分。

嗯, 和Ptype保持一致会更好点

infdahai · 2023-06-07T12:18:08Z

src/storage/src/storage.cc

@@ -1291,6 +1287,52 @@ Status Storage::Type(const std::string& key, std::string* type) {
 return Status::OK();
 }

+Status Storage::Type(const std::string& key, std::vector<std::string>& types) {


Storage::Type(const std::string& key, std::string* type)函数中与该函数操作类似，可以考虑包装下这个循环。

你的意思是循环调用 Storage::Type(const std::string& key, std::string* type) 这个函数吗, 但是这个函数每次遇到第一个符合的就return了. 这个没想通, 还请再提示一下

和这个函数相比

pika/src/storage/src/storage.cc

Line 1241 in 5ce68e4

Status Storage::Type(const std::string& key, std::string* type) {

这里绝大部分的逻辑都是通用的。我们可以把这个逻辑包装成某个统一的内部函数，然后用Type 和 Ptype 函数内部调用它。来避免产生重复代码

infdahai · 2023-06-09T06:56:10Z

src/pika_command.cc

@@ -176,6 +176,9 @@ void InitCmdTable(CmdTable* cmd_table) {
 ////TypeCmd
 std::unique_ptr<Cmd> typeptr = std::make_unique<TypeCmd>(kCmdNameType, 2, kCmdFlagsRead | kCmdFlagsSingleSlot | kCmdFlagsKv);
 cmd_table->insert(std::pair<std::string, std::unique_ptr<Cmd>>(kCmdNameType, std::move(typeptr)));
+ ////PTypeCmd
+ std::unique_ptr<Cmd> pTypeptr = std::make_unique<PTypeCmd>(kCmdNamePType, 2, kCmdFlagsRead | kCmdFlagsSingleSlot | kCmdFlagsKv);
+ cmd_table->insert(std::pair<std::string, std::unique_ptr<Cmd>>(kCmdNamePType, std::move(pTypeptr)));
 ////ScanCmd
 std::unique_ptr<Cmd> scanptr = std::make_unique<ScanCmd>(kCmdNameScan, -2, kCmdFlagsRead | kCmdFlagsMultiSlot | kCmdFlagsKv);
 cmd_table->insert(std::pair<std::string, std::unique_ptr<Cmd>>(kCmdNameScan, std::move(scanptr)));


这里按gpt的要求使用emplace吧

infdahai · 2023-06-09T06:57:07Z

src/storage/src/storage.cc

- current_task_type_(kNone),
- bg_tasks_should_exit_(false),
- scan_keynum_exit_(false) {
+ : is_opened_(false), current_task_type_(kNone), bg_tasks_should_exit_(false), scan_keynum_exit_(false) {


这里默认构造函数参数设置更推荐在定义class的时候赋值。

像这些是因为在commit的时候自动格式化代码了, 我就没管,提交了, 这样的代码还有挺多的, 我更倾向于统一在一个pr里做这些事

嗯。可以在提交个pr关于这个的。

在 class 内复初值这事已经做完了，后续不会再有统一的大 pr 了。如果发现有遗漏，尽量在相关pr内解决掉

OK 那就在这里一起改了

infdahai · 2023-06-09T07:05:09Z

src/storage/src/storage.cc

@@ -1291,6 +1287,52 @@ Status Storage::Type(const std::string& key, std::string* type) {
 return Status::OK();
 }

+Status Storage::Type(const std::string& key, std::vector<std::string>& types) {


和这个函数相比

pika/src/storage/src/storage.cc

Line 1241 in 5ce68e4

Status Storage::Type(const std::string& key, std::string* type) {

这里绝大部分的逻辑都是通用的。我们可以把这个逻辑包装成某个统一的内部函数，然后用Type 和 Ptype 函数内部调用它。来避免产生重复代码

cr-gpt · 2023-06-10T07:53:23Z

src/pika_kv.cc

@@ -1376,8 +1394,7 @@ void PKRScanRangeCmd::Do(std::shared_ptr<Slot> slot) {
 std::string next_key;
 std::vector<std::string> keys;
 std::vector<storage::KeyValue> kvs;
- rocksdb::Status s =
- slot->db()->PKRScanRange(type_, key_start_, key_end_, pattern_, limit_, &keys, &kvs, &next_key);
+ rocksdb::Status s = slot->db()->PKRScanRange(type_, key_start_, key_end_, pattern_, limit_, &keys, &kvs, &next_key);

 if (s.ok()) {
 res_.AppendArrayLen(2);


The code patch is mostly related to Redis commands implementation using RocksDB. Here are a few improvements and bug risk suggestions:

The code follows the Google C++ Style Guide.

Some methods can have more meaningful names for clarity, such as DoInitial() to Initialize().

When checking vector sizes, it is better to use the unsigned type size_t instead of signed types to avoid unexpected behavior when the vector is empty or negative indices are used.

There is inconsistent spacing in between code blocks. An IDE might flag this inconsistency as a warning.

It is recommended to ensure that any data passed through res_ is sanitized before being sent to the user to prevent any malicious attacks like cross-site scripting (XSS) attacks.

There is no handling of the case where an input string parameter contains null bytes \0, which might lead to false interpretation of the rest of the string if it is based on C-style libraries.

It is better to pass variables by constant references instead of by value to reduce copies and speed up the function.

There is a possible problem related to security when passing a user entered pattern directly to the underlying storage system without escaping or sanitizing the pattern first. If the pattern entered by the user has special characters that have meaning (such as regular expression characters), they could cause unintended results.

Note: This is a non-exhaustive list of suggestions and improvements and does not replace a thorough code review.

infdahai · 2023-06-11T10:34:39Z

src/storage/src/storage.cc

 } else if (!s.IsNotFound()) {
 return s;
 }
+ if (types.capacity() == 1 && !types.empty()) {


不要用capacity，写个lambda函数来判断条件吧。

auto isSingle = [&]()->bool{ return types.size() == 1; };

用lambda确实会好点,这个之前我没有想到, 但是判断是否是single需要用capacity, 因为在type 和ptype函数`里通过初始化不同大小的vector来判断是否查询单个

Ptype https:/OpenAtomFoundation/pika/pull/1586/files#diff-d6dd7e308aeb063b169e0d1f3e78798169c9662b0003bf34d13460fce0eb4f08R1083-R1085

Type https:/OpenAtomFoundation/pika/pull/1586/files#diff-d6dd7e308aeb063b169e0d1f3e78798169c9662b0003bf34d13460fce0eb4f08R1065-R1067

这里调用的是 explicit vector( size_type count ); 来源于 https://en.cppreference.com/w/cpp/container/vector/vector

Status Storage::GetType(const std::string& key, std::vector<std::string>& types) { types.clear(); Status s; .............

这个函数进来后会先clear 一下, 这样长度不就又恢复成0了吗, 如果没有clear, 后面调用 emplace_back 会再次扩容了

emmm，那这里我觉得添加一个enum参数来判断传入的是哪个类型。不要用capacity函数。你觉得呢

原来我是想加一个bool类型的变量来区分查多个还是单个, 后来一想可以用初始容量来判断, 就直接用了, 不知道你觉得不要用capacity 函数的原因是什么呢

当我们处理具体数量的时候应该使用size()， capacity更多是在我们关注内存容量和性能提升的时候考虑。

还是在参数里加一个变量吧, 这样lambda 也能省掉, 看着也更清晰

cr-gpt · 2023-06-11T12:33:23Z

src/pika_kv.cc

@@ -1376,8 +1394,7 @@ void PKRScanRangeCmd::Do(std::shared_ptr<Slot> slot) {
 std::string next_key;
 std::vector<std::string> keys;
 std::vector<storage::KeyValue> kvs;
- rocksdb::Status s =
- slot->db()->PKRScanRange(type_, key_start_, key_end_, pattern_, limit_, &keys, &kvs, &next_key);
+ rocksdb::Status s = slot->db()->PKRScanRange(type_, key_start_, key_end_, pattern_, limit_, &keys, &kvs, &next_key);

 if (s.ok()) {
 res_.AppendArrayLen(2);


Overall, the code patch looks fine with no obvious bugs or risks. Here are some improvements/suggestions:

The commits could benefit from more descriptive messages to help with code maintenance and collaboration.

The code formatting could be made more consistent (such as indentation, spacing).

For TypeCmd and PTypeCmd, it might be helpful to use a switch-case statement for the string types instead of using an array of strings.

cr-gpt · 2023-06-11T12:33:33Z

src/storage/include/storage/storage.h

@@ -1019,7 +1019,7 @@ class Storage {

 Status SetOptions(const OptionType& option_type, const std::string& db_type,
 const std::unordered_map<std::string, std::string>& options);
- void GetRocksDBInfo(std::string &info);
+ void GetRocksDBInfo(std::string& info);

 private:
 std::unique_ptr<RedisStrings> strings_db_;


Overall, the code changes appear to be minor and mostly related to formatting. However, there are a couple of observations:

In the BGTask constructor, there is a typo in the parameter name _opeation, it should be _operation.

In the Storage class, the GetType method's signature and implementation do not match. The signature expects a reference to a vector of strings for the types parameter, while the implementation expects a pointer to a string.

There seems to be a typo in the comment for the GetRocksDBInfo method - "RocksDB" is misspelled as "RocksdDB".

Apart from these observations, no significant bug risks or improvement suggestions stand out from the code patch.

infdahai · 2023-06-13T01:41:33Z

LGTM

AlexStocks · 2023-06-13T14:29:31Z

这个命令能否添加一些测试啊？

lqxhub · 2023-06-14T01:01:21Z

这个命令能否添加一些测试啊？

用tcl写测试吗等我周末学一下之前没接触过😁

AlexStocks · 2023-06-14T12:30:04Z

这个命令能否添加一些测试啊？

用tcl写测试吗等我周末学一下之前没接触过😁

参照 https:/OpenAtomFoundation/pika/pull/1582/files 这个 pr 的例子加下，tcl 不是非常麻烦哈

infdahai · 2023-06-16T03:49:51Z

完成pr后可以更新下wiki

Co-authored-by: liuyuecai <[email protected]>

…penAtomFoundation#1590) * fix memory leaks in PikaReplBgWorker::HandleBGWorkerWriteDB(void* arg) and PikaReplClientConn::HandleMetaSyncResponse(void* arg). * add address/thread sanitizer to CMakeLists --------- Co-authored-by: cjh <[email protected]>

)

…penAtomFoundation#1595) using unique_ptr.reset to fix the SIGABRT error && using time-wait lock to notify quickly

…1596)

* fix g++15 compile failure * add rocksdb dependency to pstd --------- Co-authored-by: J1senn <[email protected]>

* ci: add unit test in github workflow

cr-gpt · 2023-06-18T08:28:02Z

tests/test_helper.tcl

+# because the comment not works in tcl list, use regsub to ignore the item starting with '#'
+regsub -all {#.*?\n} $::all_tests {} ::all_tests
+
+
 # Index to the next test to run in the ::all_tests list.
 set ::next_test 0



Overall, the code appears to be a test suite that specifies which tests to run. Here are some observations and suggestions:

The code comments out several items from the ::all_tests list. You may want to check if these tests are still relevant and why they are commented out. If they are unnecessary, it's better to remove them entirely.

The regsub command removes items starting with "#" from the ::all_tests list. This approach makes sense considering that Tcl doesn't support inline comments in lists. However, the regex pattern could be updated to remove the entire line rather than just the comment, to avoid empty elements in the list.

It would be helpful to include more context about how the ::all_tests list is used in the code so that we can provide feedback on its efficiency and effectiveness.

There are no obvious bug risks, but it's difficult to provide a full review without seeing the rest of the codebase.

cr-gpt · 2023-06-18T08:28:09Z

tests/unit/type.tcl

+ r sadd key1 key1
+ assert_equal {string hash list zset set} [r ptype key1]
+ }
+}


This code patch appears to be testing the functionality of Redis commands related to data types. Here are a few suggestions and improvements that could be made:

Add comments to the code to make it more clear what each test is doing and why

Use descriptive variable names for clarity

Consider setting up different contexts for each test, so they do not need to rely on each other's state

Add additional tests to ensure edge cases, such as empty or null values, are handled correctly

As far as potential bugs go, it's difficult to determine without knowing more about the context and intended usage of this code.

cr-gpt · 2023-06-19T11:39:21Z

src/storage/include/storage/storage.h


 // For scan keys in data base
- std::atomic<bool> scan_keynum_exit_;
+ std::atomic<bool> scan_keynum_exit_ = false;
 };

 } // namespace storage


Here are my observations and suggestions:

In the constructor of BGTask, _opeation should be _operation.

The default value for _argv parameter in the constructor of BGTask has been misspelled as _opeation.

The method signature for GetType should be modified to return a vector of strings as its return type.

is_opened_, bg_tasks_thread_id_, current_task_type_, and bg_tasks_should_exit_ have been assigned default values, but these variables can be initialized with initialization lists instead of using assignment statements.

The spelling of Reutrns in GetType and Type should be corrected to "Returns."

It's unclear what the purpose of cursors_store_ is. Its name doesn't suggest its use, and it's not used anywhere in the provided code patch. Consider renaming it or removing it altogether.

The use of pthread_t to manage background tasks is not recommended. C++11 introduced std::thread, which provides a simpler interface for managing threads. Consider migrating to std::thread.

The comment for GetRocksDBInfo mentions info as an input parameter, but in reality, it's an output parameter. Consider updating the comment for clarity.

Overall, the code seems reasonable, and I don't see any obvious bug risks.

cr-gpt · 2023-06-19T11:39:28Z

src/storage/src/storage.cc

+ hashes_db_->GetRocksDBInfo(info, "hashes_");
+ lists_db_->GetRocksDBInfo(info, "lists_");
+ sets_db_->GetRocksDBInfo(info, "sets_");
+ zsets_db_->GetRocksDBInfo(info, "zsets_");
 }

 } // namespace storage


The code changes seem reasonable and there are no obvious bugs or errors. Here are a few minor suggestions for improvement:

In the constructor of Storage, it might be worth initializing all member variables in an initialization list rather than in the constructor body.

The function Storage::PfAdd() could benefit from some error handling if the input vector contains empty strings or null pointers.

In the function Storage::GetType(), when single is true and the result vector is not empty, it seems that you could skip running the remaining checks since only one type is needed.

Other than these small points, the code looks concise and well-structured.

infdahai · 2023-07-01T15:01:57Z

LGTM

cr-gpt · 2023-07-01T15:24:10Z

include/pika_command.h

@@ -100,6 +100,7 @@ const std::string kCmdNameTtl = "ttl";
 const std::string kCmdNamePttl = "pttl";
 const std::string kCmdNamePersist = "persist";
 const std::string kCmdNameType = "type";
+const std::string kCmdNamePType = "ptype";
 const std::string kCmdNameScan = "scan";
 const std::string kCmdNameScanx = "scanx";
 const std::string kCmdNamePKSetexAt = "pksetexat";


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

A new constant variable kCmdNamePType has been added.

Improvement suggestion:

It's generally good practice to follow consistent naming conventions for variables. Consider using the same naming convention as other variables (e.g., camelCase or snake_case).

Bug risk assessment:

As this is just a constant string addition, there is a low risk of introducing bugs within this change alone.

Overall, this code patch seems relatively safe and straightforward, with only a minor improvement suggestion regarding naming conventions.

cr-gpt · 2023-07-01T15:24:16Z

src/pika_command.cc

@@ -280,6 +280,9 @@ void InitCmdTable(CmdTable* cmd_table) {
 std::unique_ptr<Cmd> typeptr =
 std::make_unique<TypeCmd>(kCmdNameType, 2, kCmdFlagsRead | kCmdFlagsSingleSlot | kCmdFlagsKv);
 cmd_table->insert(std::pair<std::string, std::unique_ptr<Cmd>>(kCmdNameType, std::move(typeptr)));
+ ////PTypeCmd
+ std::unique_ptr<Cmd> pTypeptr = std::make_unique<PTypeCmd>(kCmdNamePType, 2, kCmdFlagsRead | kCmdFlagsSingleSlot | kCmdFlagsKv);
+ cmd_table->insert(std::pair<std::string, std::unique_ptr<Cmd>>(kCmdNamePType, std::move(pTypeptr)));
 ////ScanCmd
 std::unique_ptr<Cmd> scanptr =
 std::make_unique<ScanCmd>(kCmdNameScan, -2, kCmdFlagsRead | kCmdFlagsMultiSlot | kCmdFlagsKv);


Based on the provided code patch, here is a brief code review:

It appears that you're inserting a new command, PTypeCmd, into the cmd_table.

Make sure that the PTypeCmd class has been defined and implemented correctly.

Ensure that the constructor parameters, number of arguments (-2), and flags are appropriate for this command.

Other parts of the code seem fine at first glance, but without additional context or the complete code, it's hard to provide more specific feedback.

Some general suggestions for improvement:

Review the overall design and structure of the code to ensure it follows best practices and provides good readability, maintainability, and modularity.

Consider adding comments to describe the purpose and functionality of different sections or classes in the code.

Verify if there are any error handling mechanisms or exception handling for potential exceptions that may occur during command execution.

Remember to thoroughly test the modified code to ensure it behaves as expected and doesn't introduce any bugs or issues.

cr-gpt · 2023-07-01T15:24:26Z

src/pika_kv.cc

@@ -1328,8 +1347,7 @@ void PKScanRangeCmd::Do(std::shared_ptr<Slot> slot) {
 std::string next_key;
 std::vector<std::string> keys;
 std::vector<storage::KeyValue> kvs;
- rocksdb::Status s =
- slot->db()->PKScanRange(type_, key_start_, key_end_, pattern_, limit_, &keys, &kvs, &next_key);
+ rocksdb::Status s = slot->db()->PKScanRange(type_, key_start_, key_end_, pattern_, limit_, &keys, &kvs, &next_key);

 if (s.ok()) {
 res_.AppendArrayLen(2);


Overall, the code patch seems to involve mostly formatting changes and a few small modifications. Here are some potential bug risks and improvement suggestions for the code:

DelCmd::Merge() and ExistsCmd::Merge(): The original code includes multiple lines for merging, but the modified code combines them into a single line. Since the implementation has been reduced, it's important to verify that the intended functionality is still correct.

TypeCmd::Do(): In the modified code, GetType() method is used with the flag true, which retrieves the type as a single string in the "types" vector. However, the original code simply retrieved the type as a string directly. Ensure that the change doesn't affect the desired behavior of the code.

PTypeCmd class: It seems to be a new addition to the code patch. The implementation scans the types of keys using GetType() with different flags and adds the results to the response. Review the new functionality carefully and ensure it aligns with your requirements.

PKScanRangeCmd::Do(): The modified code no longer spans multiple lines for calling PKScanRange(); however, it's generally a good practice to maintain more readable formatting, particularly if the method accepts multiple arguments. Verify if the updated line is still readable and understandable.

In general, consider adding comments or documentation to describe the purpose and functionality of the methods and classes, especially for newly added code.

Please note that this review is based solely on the provided code patch, and other parts of the codebase might also require review and consideration.

* fix g++15 compile failure * add rocksdb dependency to pstd --------- Co-authored-by: J1senn <[email protected]>

* test: optimize the return results of srandmember to avoid approximate results * fix: use last_seed for random engine

cr-gpt · 2023-07-02T02:58:41Z

src/pika_migrate_thread.cc

 return 0;
 } else {
- LOG(WARNING) << "PikaMigrateThread::ReqMigrateOne key: " << key << " type: " << type_str << " is illegal";
+ LOG(WARNING) << "PikaMigrateThread::ReqMigrateOne key: " << key << " type: " << type_str[0] << " is illegal";
 return -1;
 }



Here are some observations and suggestions regarding the provided code patch:

In the original code, std::string type_str is used to store the value returned by slot->db()->Type(key, &type_str), but in the patch, it is changed to std::vector<std::string> type_str(1). It seems unnecessary to use a vector here since only a single value is required. You can revert it back to std::string type_str.

The method GetType is used instead of Type, and an additional parameter true is passed. Check if this change in behavior is expected and documented, as it could affect the logic or results.

Instead of comparing type_str directly to string literals like "string" or "hash", the patch compares type_str[0] to these values. This assumes that type_str will always contain at least one element. Ensure that this assumption is valid, and consider adding sufficient error handling if it's possible for type_str to be empty.

While handling the case when type_str is illegal, the patch logs type_str[0] instead of type_str in the warning message. Confirm if this change is intentional, or if logging the entire type_str is more useful for debugging.

Overall, the provided code patch looks fine with the assumption that the changes made are intentional and aligned with the desired behavior.

cr-gpt · 2023-07-02T02:58:52Z

src/pika_slot_command.cc

 key_type_ = 's';
- } else if (type_str == "zset") {
+ } else if (type_str[0] == "zset") {
 key_type_ = 'z';
 } else {
 LOG(WARNING) << "Migrate slot key: " << key_ << " not found";


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

In the original code, rocksdb::Status s = slot->db()->Type(key, &type_str); is used to get the type of a key, while in the patch it has been replaced with rocksdb::Status s = slot->db()->GetType(key, true, type_str); which uses a vector of strings instead of a single string. This change might indicate that the GetType function now returns multiple types for a given key. If this is the intended behavior, then the code changes are fine.

In both the GetKeyType and SlotsMgrtTagOneCmd::KeyTypeCheck functions, the updated code compares type_str[0] with the key types ("string", "hash", "list", etc.) instead of directly comparing type_str itself. This indicates that type_str is now an array (vector) containing the type(s) of a key. Again, if this is the intended behavior, then the changes are correct.

It seems like the RemSlotKey function has not been included in the code patch you provided. Make sure this function is properly updated, if necessary, based on the changes made in other parts of the code.

It's important to review the implementation of the GetType function itself since it determines the behavior of getting the key type. Ensure that the function is correctly implemented and handles any potential errors or edge cases.

Overall, from the code patch you provided, it appears that the changes made relate to modifying the way key types are stored and retrieved. The actual correctness of these changes depends on the implementation details of the GetType function and the desired behavior of the code.

cr-gpt · 2023-07-02T02:58:57Z

tests/test_helper.tcl

 unit/command
+ unit/type
 }

 # because the comment not works in tcl list, use regsub to ignore the item starting with '#'


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

There are duplicate entries for unit/type in the ::all_tests list. This could be an oversight and might lead to duplicate testing or unintended behavior.

Improvement suggestion:

Remove one of the duplicate entries for unit/type to avoid duplication and potential confusion.

Please note that without a more comprehensive understanding of the codebase, it is difficult to identify other potential bug risks or provide further improvement suggestions. It's always recommended to thoroughly test any changes and conduct a detailed code review with respect to the specific requirements and design patterns used in your project.

cr-gpt · 2023-07-02T03:50:19Z

src/pika_kv.cc

@@ -1328,8 +1347,7 @@ void PKScanRangeCmd::Do(std::shared_ptr<Slot> slot) {
 std::string next_key;
 std::vector<std::string> keys;
 std::vector<storage::KeyValue> kvs;
- rocksdb::Status s =
- slot->db()->PKScanRange(type_, key_start_, key_end_, pattern_, limit_, &keys, &kvs, &next_key);
+ rocksdb::Status s = slot->db()->PKScanRange(type_, key_start_, key_end_, pattern_, limit_, &keys, &kvs, &next_key);

 if (s.ok()) {
 res_.AppendArrayLen(2);


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

In the DelCmd and ExistsCmd classes, the Merge() function implementation has been simplified to a single line. This change seems fine as long as it doesn't affect any existing functionality.

In the TypeCmd class, the Do function has been modified to use the GetType method with a new parameter (true). Without knowing the details of your codebase and requirements, it's difficult to determine if this change is appropriate. Ensure that this change doesn't introduce any unintended side effects or break existing functionality.

In the PTypeCmd class, a new command PType has been added, which fetches multiple types of a given key using the GetType method. The results are appended to the response in an array format. This implementation looks fine, assuming it aligns with your desired behavior.

In the PKScanRangeCmd class, the Do function has been rewritten to call the PKScanRange method with updated arguments. This change seems reasonable, but ensure that it doesn't introduce any unintended behavior or break existing functionality.

It's challenging to conduct a comprehensive code review based on a small code patch. It's crucial to consider the context, purpose, and requirements of the code before making any changes. Additionally, testing and validating the modified code is essential to ensure it functions correctly and doesn't introduce regressions.

* feat: supported 'ptype' command * Simplified command `type` and `ptype` code * Remove the capacity judgment * fix slavaof serialize response bug (OpenAtomFoundation#1583) Co-authored-by: liuyuecai <[email protected]> * fix (OpenAtomFoundation#1587) (OpenAtomFoundation#1588) * fix unit test:type/set (OpenAtomFoundation#1577) * Fix memory leaks in HandleBGWorkerWriteDB and HandleMetaSyncResponse (OpenAtomFoundation#1590) * fix memory leaks in PikaReplBgWorker::HandleBGWorkerWriteDB(void* arg) and PikaReplClientConn::HandleMetaSyncResponse(void* arg). * add address/thread sanitizer to CMakeLists --------- Co-authored-by: cjh <[email protected]> * refactor: replace pstd/env with std::filesystem (OpenAtomFoundation#1470) * fix bug issue 1554: using unique_ptr.reset to fix the SIGABRT error (OpenAtomFoundation#1595) using unique_ptr.reset to fix the SIGABRT error && using time-wait lock to notify quickly * fix: incorrect manner of terminating the process (OpenAtomFoundation#1596) * fix issue#1597: add rocksdb dependency to pstd (OpenAtomFoundation#1599) * fix g++15 compile failure * add rocksdb dependency to pstd --------- Co-authored-by: J1senn <[email protected]> * fix_info_command * fix: gcc13 compile failed (OpenAtomFoundation#1601) * ci: add unit test to github action (OpenAtomFoundation#1604) * ci: add unit test in github workflow * chore:change `PLATFORM` field logic (OpenAtomFoundation#1615) * fix issue 1517: scan 命令不支持 type 参数 (OpenAtomFoundation#1582) * fix: the unit test of type/set (OpenAtomFoundation#1617) * test: optimize the return results of srandmember to avoid approximate results * fix: use last_seed for random engine * [fix issue1621] fix deadlock (OpenAtomFoundation#1620) * [fix] fix deadlock * [fix] fix * command `type` and `ptype` add unit test * fix member variable initialization * Update issue templates * feat: supported 'ptype' command * fix unit test:type/set (OpenAtomFoundation#1577) * fix issue#1597: add rocksdb dependency to pstd (OpenAtomFoundation#1599) * fix g++15 compile failure * add rocksdb dependency to pstd --------- Co-authored-by: J1senn <[email protected]> * fix: gcc13 compile failed (OpenAtomFoundation#1601) * fix: the unit test of type/set (OpenAtomFoundation#1617) * test: optimize the return results of srandmember to avoid approximate results * fix: use last_seed for random engine * Modify other modules to use the new `type` function * fix function repeat --------- Co-authored-by: Yuecai Liu <[email protected]> Co-authored-by: liuyuecai <[email protected]> Co-authored-by: Peter Chan <[email protected]> Co-authored-by: chenbt <[email protected]> Co-authored-by: Junhua Chen <[email protected]> Co-authored-by: cjh <[email protected]> Co-authored-by: kang jinci <[email protected]> Co-authored-by: Xin.Zh <[email protected]> Co-authored-by: machinly <[email protected]> Co-authored-by: A2ureStone <[email protected]> Co-authored-by: J1senn <[email protected]> Co-authored-by: chejinge <[email protected]> Co-authored-by: baerwang <[email protected]> Co-authored-by: ptbxzrt <[email protected]> Co-authored-by: 你不要过来啊 <[email protected]>

feat: supported 'ptype' command

c67ac2f

cr-gpt bot reviewed Jun 4, 2023

View reviewed changes

infdahai reviewed Jun 7, 2023

View reviewed changes

infdahai reviewed Jun 9, 2023

View reviewed changes

Simplified command type and ptype code

9ec9485

cr-gpt bot reviewed Jun 10, 2023

View reviewed changes

infdahai reviewed Jun 11, 2023

View reviewed changes

Remove the capacity judgment

99a97b2

cr-gpt bot reviewed Jun 11, 2023

View reviewed changes

luky116 and others added 12 commits June 18, 2023 16:27

fix slavaof serialize response bug (OpenAtomFoundation#1583)

ed7cca2

Co-authored-by: liuyuecai <[email protected]>

fix (OpenAtomFoundation#1587) (OpenAtomFoundation#1588)

e691134

fix unit test:type/set (OpenAtomFoundation#1577)

e0bdee6

refactor: replace pstd/env with std::filesystem (OpenAtomFoundation#1470

424fb93

)

fix bug issue 1554: using unique_ptr.reset to fix the SIGABRT error (O…

9db60db

…penAtomFoundation#1595) using unique_ptr.reset to fix the SIGABRT error && using time-wait lock to notify quickly

fix: incorrect manner of terminating the process (OpenAtomFoundation#…

2b7032f

…1596)

fix issue#1597: add rocksdb dependency to pstd (OpenAtomFoundation#1599)

3f577ff

* fix g++15 compile failure * add rocksdb dependency to pstd --------- Co-authored-by: J1senn <[email protected]>

fix_info_command

b7ecdd0

fix: gcc13 compile failed (OpenAtomFoundation#1601)

34c5de2

ci: add unit test to github action (OpenAtomFoundation#1604)

336c931

* ci: add unit test in github workflow

chore:change PLATFORM field logic (OpenAtomFoundation#1615)

376c70f

cr-gpt bot reviewed Jun 18, 2023

View reviewed changes

lqxhub added 2 commits June 18, 2023 20:10

Merge branch 'unstable' into ptype

4f5314e

fix member variable initialization

23ff105

cr-gpt bot reviewed Jun 19, 2023

View reviewed changes

AlexStocks previously approved these changes Jul 1, 2023

View reviewed changes

Merge branch 'unstable' into ptype

0888591

AlexStocks dismissed their stale review via 0888591 July 1, 2023 15:24

cr-gpt bot reviewed Jul 1, 2023

View reviewed changes

AlexStocks and others added 7 commits July 2, 2023 10:54

Update issue templates

7bf5afb

feat: supported 'ptype' command

016d6fe

fix unit test:type/set (OpenAtomFoundation#1577)

b14c3fd

fix issue#1597: add rocksdb dependency to pstd (OpenAtomFoundation#1599)

d110dc6

* fix g++15 compile failure * add rocksdb dependency to pstd --------- Co-authored-by: J1senn <[email protected]>

fix: gcc13 compile failed (OpenAtomFoundation#1601)

e480f24

fix: the unit test of type/set (OpenAtomFoundation#1617)

6072bc0

* test: optimize the return results of srandmember to avoid approximate results * fix: use last_seed for random engine

Modify other modules to use the new type function

2f3db19

cr-gpt bot reviewed Jul 2, 2023

View reviewed changes

fix function repeat

da9b876

cr-gpt bot reviewed Jul 2, 2023

View reviewed changes

AlexStocks approved these changes Jul 2, 2023

View reviewed changes

AlexStocks merged commit 41965db into OpenAtomFoundation:unstable Jul 2, 2023

lqxhub deleted the ptype branch December 7, 2023 14:59

feat: supported 'ptype' command #1586

feat: supported 'ptype' command #1586

Conversation

lqxhub commented Jun 4, 2023

cr-gpt bot Jun 4, 2023

Choose a reason for hiding this comment

cr-gpt bot Jun 4, 2023

Choose a reason for hiding this comment

cr-gpt bot Jun 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqxhub commented Jun 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infdahai Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infdahai Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

cr-gpt bot Jun 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infdahai Jun 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infdahai Jun 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cr-gpt bot Jun 11, 2023

Choose a reason for hiding this comment

cr-gpt bot Jun 11, 2023

Choose a reason for hiding this comment

infdahai commented Jun 13, 2023

AlexStocks commented Jun 13, 2023

lqxhub commented Jun 14, 2023

AlexStocks commented Jun 14, 2023

infdahai commented Jun 16, 2023

cr-gpt bot Jun 18, 2023

Choose a reason for hiding this comment

cr-gpt bot Jun 18, 2023

Choose a reason for hiding this comment

cr-gpt bot Jun 19, 2023

Choose a reason for hiding this comment

cr-gpt bot Jun 19, 2023

Choose a reason for hiding this comment

infdahai commented Jul 1, 2023

cr-gpt bot Jul 1, 2023

Choose a reason for hiding this comment

cr-gpt bot Jul 1, 2023

Choose a reason for hiding this comment

cr-gpt bot Jul 1, 2023

Choose a reason for hiding this comment

cr-gpt bot Jul 2, 2023

Choose a reason for hiding this comment

cr-gpt bot Jul 2, 2023

Choose a reason for hiding this comment

cr-gpt bot Jul 2, 2023

Choose a reason for hiding this comment

cr-gpt bot Jul 2, 2023

Choose a reason for hiding this comment

infdahai Jun 9, 2023 •

edited

Loading

infdahai Jun 9, 2023 •

edited

Loading

infdahai Jun 11, 2023 •

edited

Loading

infdahai Jun 11, 2023 •

edited

Loading