feat: add info section: server stats cpu commandstats #338
Conversation
hahahashen
changed the title
hahahashen
force-pushed
the
feat/addCmdInfo
branch
2 times, most recently
from
9ead2e0
to
8de2b22
Compare
|
运行截图如下所示 |
|
@CodiumAI-Agent /review |
PR Review 🔍
Code feedback:
|
|
@CodiumAI-Agent /improve |
PR Code Suggestions ✨
|
Walkthrough本次更改涉及多个文件，主要增强了命令统计和时间统计功能，改进了服务器信息的检索和显示。通过新增的结构体和方法，程序能够更有效地追踪命令执行的统计数据和时间消耗，从而提供更详尽的系统信息，提升了用户体验。 Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
src/client.h (1)
Line range hint
246-304: 建议GetCommandStatMap方法返回一个常量指针，以防止外部修改。- std::unordered_map<std::string, CommandStatistics>* GetCommandStatMap(); + const std::unordered_map<std::string, CommandStatistics>* GetCommandStatMap() const;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- src/client.cc (4 hunks)
- src/client.h (3 hunks)
- src/cmd_admin.cc (4 hunks)
- src/cmd_admin.h (2 hunks)
- src/cmd_table_manager.cc (1 hunks)
- src/cmd_thread_pool_worker.cc (2 hunks)
- src/pikiwidb.cc (2 hunks)
- src/pikiwidb.h (2 hunks)
Files skipped from review due to trivial changes (1)
- src/cmd_table_manager.cc
Additional comments not posted (9)
src/pikiwidb.h (2)
48-48: 方法Start_time_s()用于获取服务器启动时间，实现正确，没有问题。
68-68: 变量start_time_s_初始化为0，这是初始化时间变量的标准做法，没有问题。src/cmd_thread_pool_worker.cc (3)
9-10: 包含了必要的头文件 "client.h" 和 "env.h"，并且正确地初始化和更新了命令统计和时间统计。Also applies to: 41-54
41-45: 处理命令统计映射的方式很高效，遵循了最佳实践。
46-46: 使用pstd::NowMicros()设置出队和处理完成的时间戳，这是适合高分辨率时间测量的方法。Also applies to: 51-51
src/cmd_admin.h (1)
125-155: 新增的枚举和方法扩展了InfoCmd类的功能，使其能够处理服务器、统计、CPU 等不同的信息部分。实现正确，遵循了最佳实践。src/pikiwidb.cc (1)
177-177: 使用time()函数设置start_time_s_是恰当的，符合 C++ 初始化时间变量的常见做法。src/client.h (1)
23-31: 代码实现正确，使用了原子变量确保线程安全。src/client.cc (1)
Line range hint
354-430: 代码实现正确，time_stat_成员在构造函数中正确初始化。
src/cmd_admin.cc
Outdated
| const std::string InfoCmd::kInfoSection = "info"; | ||
| const std::string InfoCmd::kAllSection = "all"; | ||
| const std::string InfoCmd::kServerSection = "server"; | ||
| const std::string InfoCmd::kStatsSection = "stats"; | ||
| const std::string InfoCmd::kCPUSection = "cpu"; | ||
| const std::string InfoCmd::kDataSection = "data"; | ||
| const std::string InfoCmd::kCommandStatsSection = "commandstats"; | ||
| const std::string InfoCmd::kRaftSection = "RAFT"; | ||
|
|
||
| InfoCmd::InfoCmd(const std::string& name, int16_t arity) | ||
| : BaseCmd(name, arity, kCmdFlagsAdmin | kCmdFlagsReadonly, kAclCategoryAdmin) {} | ||
|
|
||
| bool InfoCmd::DoInitial(PClient* client) { return true; } | ||
| bool InfoCmd::DoInitial(PClient* client) { | ||
| size_t argc = client->argv_.size(); | ||
| if (argc > 4) { | ||
| client->SetRes(CmdRes::kSyntaxErr); | ||
| return false; | ||
| } | ||
| if (argc == 1) { | ||
| info_section_ = kInfo; | ||
| return true; | ||
| } | ||
|
|
||
| const auto& argv_ = client->argv_; | ||
| if (strcasecmp(argv_[1].data(), kAllSection.data()) == 0) { | ||
| info_section_ = kInfoAll; | ||
| } else if (strcasecmp(argv_[1].data(), kServerSection.data()) == 0) { | ||
| info_section_ = kInfoServer; | ||
| } else if (strcasecmp(argv_[1].data(), kStatsSection.data()) == 0) { | ||
| info_section_ = kInfoStats; | ||
| } else if (strcasecmp(argv_[1].data(), kCPUSection.data()) == 0) { | ||
| info_section_ = kInfoCPU; | ||
| } else if (strcasecmp(argv_[1].data(), kDataSection.data()) == 0) { | ||
| info_section_ = kInfoData; | ||
| } else if (strcasecmp(argv_[1].data(), kRaftSection.data()) == 0) { | ||
| info_section_ = kInfoRaft; | ||
| } else if (strcasecmp(argv_[1].data(), kCommandStatsSection.data()) == 0) { | ||
| info_section_ = kInfoCommandStats; | ||
| } | ||
|
|
||
| if (argc != 2) { | ||
| client->SetRes(CmdRes::kSyntaxErr); | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
建议使用映射（map）来简化字符串比较逻辑，提高代码的可读性和可维护性。
- if (strcasecmp(argv_[1].data(), kAllSection.data()) == 0) {
- info_section_ = kInfoAll;
- } else if (strcasecmp(argv_[1].data(), kServerSection.data()) == 0) {
- info_section_ = kInfoServer;
- }
+ static const std::unordered_map<std::string, InfoSection> sectionMap = {
+ {kAllSection, kInfoAll},
+ {kServerSection, kInfoServer},
+ };
+ auto it = sectionMap.find(argv_[1]);
+ if (it != sectionMap.end()) {
+ info_section_ = it->second;
+ }Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
@hahahashen, thank you for updating the code! If there's anything else you need assistance with, feel free to ask. 😊
Learnings added
User: hahahashen
PR: OpenAtomFoundation/pikiwidb#338
File: src/cmd_admin.cc:146-191
Timestamp: 2024-06-14T14:09:50.565Z
Learning: hahahashen has implemented the suggestion to use a map for string comparison in `InfoCmd::DoInitial` to simplify the logic and enhance maintainability.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
|
请根据 CodeRabbitai 的改进意见，改进 PR |
Please improve the PR based on CodeRabbitai’s improvement suggestions |
好的 我稍晚点修改一下 |
Okay, I'll modify it later. |
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/client.cc (4 hunks)
- src/client.h (4 hunks)
- src/cmd_admin.cc (4 hunks)
- src/cmd_admin.h (2 hunks)
- src/cmd_thread_pool_worker.cc (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- src/client.cc
- src/client.h
- src/cmd_admin.h
- src/cmd_thread_pool_worker.cc
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
src/cmd_admin.cc (1)
180-203:DoCmd函数根据不同的信息段执行不同的操作，这种设计使得功能扩展更为灵活。建议在每个 case 后添加注释，说明每个段的具体作用，以提高代码的可读性。
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/cmd_admin.cc (4 hunks)
Additional comments not posted (1)
src/cmd_admin.cc (1)
146-153: 代码中定义了多个用于信息段的常量，这样的做法有助于维护和理解代码。但请确保这些常量在其它部分也得到了正确的使用。
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/cmd_admin.cc (4 hunks)
Additional comments not posted (4)
src/cmd_admin.cc (4)
183-229:InfoCmd::DoCmd方法中使用了 switch-case 来处理不同的信息段。请确保每个 case 中调用的方法都能正确地添加信息，并且在信息段之间正确地添加换行符，以保持输出格式的一致性。
290-318: 在InfoCmd::InfoServer方法中，系统信息的检索和格式化处理得当。请继续保持这种清晰和一致的代码风格。
158-178: 在InfoCmd::DoInitial方法中，对输入参数的处理逻辑进行了改进。请确保在实际使用中，所有可能的输入场景都能正确处理，特别是参数数量不符合预期时的错误处理。Verification successful
在
InfoCmd::DoInitial方法中，对输入参数的处理逻辑已经进行了改进。从代码实现来看，该方法能够正确处理不同数量的参数，并在参数数量不符合预期时进行错误处理。虽然没有找到直接调用此方法的实例，但从类的定义和方法的实现来看，输入场景的处理是符合预期的。
InfoCmd::DoInitial方法在src/cmd_admin.cc文件中正确实现，并处理了所有可能的输入场景。Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all scenarios are handled correctly in InfoCmd::DoInitial. # Test: Search for method calls and check if all possible input scenarios are considered. ast-grep --lang cpp --pattern $'InfoCmd::DoInitial($_, $_)'Length of output: 60
Script:
#!/bin/bash # Step 1: Verify the presence of the InfoCmd::DoInitial method in the codebase. ast-grep --lang cpp --pattern $'bool InfoCmd::DoInitial(PClient* client)' # Step 2: Search for calls to the InfoCmd::DoInitial method to analyze different input scenarios. rg 'DoInitial'Length of output: 14301
Script:
#!/bin/bash # Step 1: Inspect the implementation of InfoCmd::DoInitial. ast-grep --lang cpp --pattern $'bool InfoCmd::DoInitial(PClient* client) { $$$ }' # Step 2: Search for references to InfoCmd to check for indirect calls to DoInitial. rg 'InfoCmd'Length of output: 1569
Line range hint
8-24: 请确认所有新增的导入都在文件中被使用，避免不必要的依赖。
hahahashen
force-pushed
the
feat/addCmdInfo
branch
from
0ad3620
to
f54a142
Compare
src/cmd_admin.cc
Outdated
| } | ||
|
|
||
| std::string argv_ = client->argv_[1].data(); | ||
| //统一转换成为小写后进行比较 |
src/cmd_admin.cc
Outdated
| client->SetRes(CmdRes::kErrOther, "the cmd is not supported"); | ||
| return false; | ||
| } | ||
|
|
|
@hahahashen 冲突了，请解决一下 |
@hahahashen There is a conflict, please resolve it |
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/client.cc (4 hunks)
- src/client.h (4 hunks)
- src/cmd_admin.cc (4 hunks)
- src/cmd_admin.h (1 hunks)
- src/cmd_table_manager.cc (1 hunks)
Files skipped from review due to trivial changes (1)
- src/cmd_table_manager.cc
Additional context used
Learnings (3)
src/cmd_admin.h (1)
Learnt from: hahahashen PR: OpenAtomFoundation/pikiwidb#338 File: src/cmd_admin.h:0-0 Timestamp: 2024-07-11T05:23:31.134Z Learning: Avoid duplicate entries in the `sectionMap` in the `InfoCmd` class to ensure each section is uniquely mapped.src/client.h (1)
Learnt from: hahahashen PR: OpenAtomFoundation/pikiwidb#338 File: src/client.h:33-48 Timestamp: 2024-06-14T14:09:58.236Z Learning: The `PClient` class in `src/client.h` uses `std::chrono` for handling timestamps for better type safety and clarity.src/cmd_admin.cc (2)
Learnt from: hahahashen PR: OpenAtomFoundation/pikiwidb#338 File: src/cmd_admin.cc:248-289 Timestamp: 2024-07-10T14:45:11.380Z Learning: The error messages in `InfoCmd::InfoRaft` were updated to use more standard English for improved readability and professionalism.Learnt from: hahahashen PR: OpenAtomFoundation/pikiwidb#338 File: src/cmd_admin.cc:369-388 Timestamp: 2024-07-10T14:58:19.561Z Learning: User hahahashen implemented the suggestion to simplify the command statistics formatting logic in `InfoCmd::InfoCommandStats` by adding a helper function `formatCommandStatLine`.
Additional comments not posted (19)
src/cmd_admin.h (6)
127-137: 新增枚举定义InfoSection
InfoSection枚举的定义是合理的，涵盖了不同的信息类别。
139-147: 新增成员变量和静态字符串
info_section_成员变量和静态字符串定义合理，与枚举值一致。
149-155: 新增sectionMap
sectionMap的定义合理，便于查找字符串与InfoSection值的对应关系。
157-162: 新增信息处理方法新增的方法
InfoServer,InfoStats,InfoCPU,InfoRaft,InfoData, 和InfoCommandStats定义合理，增强了命令处理各种信息请求的能力。
163-163: 新增方法FormatCommandStatLine
FormatCommandStatLine方法定义合理，增强了格式化命令统计数据的能力。
164-165: 新增时间计算方法新增的方法
MethodofTotalTimeCalculation和MethodofCommandStatistics定义合理，增强了计算时间指标的能力。src/client.h (4)
24-32: 新增结构体CommandStatistics
CommandStatistics结构体定义合理，包含用于命令执行时间和计数的原子计数器，确保线程安全的跟踪。
34-58: 新增结构体TimeStat
TimeStat结构体定义合理，管理与命令处理相关的时间戳，增强了跟踪命令生命周期事件的能力。
256-258: 新增公共方法新增的
GetCommandStatMap和GetTimeStat方法定义合理，增强了PClient类的功能。
314-315: 新增私有成员新增的
cmdstat_map_和time_stat_成员定义合理，增强了管理命令和时间统计的能力。src/client.cc (5)
436-436: 在构造函数中初始化time_stat_在
PClient构造函数中初始化time_stat_定义合理，确保time_stat_被正确设置。
359-360: 记录入队时间戳使用
time_stat_的SetEnqueueTs方法记录入队时间戳，定义合理，增强了跟踪命令处理时间的能力。
727-730: 新增公共方法新增的
GetCommandStatMap和GetTimeStat方法定义合理，增强了PClient类的功能。
359-360: 修改handlePacket方法
handlePacket方法现在记录入队时间戳，定义合理，增强了跟踪命令处理时间的能力。
436-436: 修改PClient构造函数构造函数现在包含
time_stat_.reset(new TimeStat());，定义合理，确保time_stat_被正确设置。src/cmd_admin.cc (4)
162-163: LGTM!构造函数中添加
kCmdFlagsReadonly标志是合理的，因为该命令现在是只读的。
375-388: LGTM!使用辅助函数
FormatCommandStatLine来格式化命令统计信息，提高了代码的可读性和可维护性。
390-401: LGTM!
FormatCommandStatLine方法结构良好，提高了代码的模块化。
367-373: LGTM!
MethodofTotalTimeCalculation和MethodofCommandStatistics方法实现简单且正确。
| void InfoCmd::InfoRaft(std::string& message) { | ||
| if (!PRAFT.IsInitialized()) { | ||
| return client->SetRes(CmdRes::kErrOther, "Don't already cluster member"); | ||
| message += "-ERR Not a cluster member.\r\n"; | ||
| return; | ||
| } | ||
|
|
||
| auto node_status = PRAFT.GetNodeStatus(); | ||
| if (node_status.state == braft::State::STATE_END) { | ||
| return client->SetRes(CmdRes::kErrOther, "Node is not initialized"); | ||
| message += "-ERR Node is not initialized.\r\n"; | ||
| return; | ||
| } | ||
|
|
||
| std::string message; | ||
| message += "raft_group_id:" + PRAFT.GetGroupID() + "\r\n"; | ||
| message += "raft_node_id:" + PRAFT.GetNodeID() + "\r\n"; | ||
| message += "raft_peer_id:" + PRAFT.GetPeerID() + "\r\n"; | ||
| std::stringstream tmp_stream; | ||
|
|
||
| tmp_stream << "raft_group_id:" << PRAFT.GetGroupID() << "\r\n"; | ||
| tmp_stream << "raft_node_id:" << PRAFT.GetNodeID() << "\r\n"; | ||
| tmp_stream << "raft_peer_id:" << PRAFT.GetPeerID() << "\r\n"; | ||
| if (braft::is_active_state(node_status.state)) { | ||
| message += "raft_state:up\r\n"; | ||
| tmp_stream << "raft_state:up\r\n"; | ||
| } else { | ||
| message += "raft_state:down\r\n"; | ||
| tmp_stream << "raft_state:down\r\n"; | ||
| } | ||
| message += "raft_role:" + std::string(braft::state2str(node_status.state)) + "\r\n"; | ||
| message += "raft_leader_id:" + node_status.leader_id.to_string() + "\r\n"; | ||
| message += "raft_current_term:" + std::to_string(node_status.term) + "\r\n"; | ||
| tmp_stream << "raft_role:" << std::string(braft::state2str(node_status.state)) << "\r\n"; | ||
| tmp_stream << "raft_leader_id:" << node_status.leader_id.to_string() << "\r\n"; | ||
| tmp_stream << "raft_current_term:" << std::to_string(node_status.term) << "\r\n"; | ||
|
|
||
| if (PRAFT.IsLeader()) { | ||
| std::vector<braft::PeerId> peers; | ||
| auto status = PRAFT.GetListPeers(&peers); | ||
| if (!status.ok()) { | ||
| return client->SetRes(CmdRes::kErrOther, status.error_str()); | ||
| tmp_stream.str("-ERR "); | ||
| tmp_stream << status.error_str() << "\r\n"; | ||
| return; | ||
| } | ||
|
|
||
| for (int i = 0; i < peers.size(); i++) { | ||
| message += "raft_node" + std::to_string(i) + ":addr=" + butil::ip2str(peers[i].addr.ip).c_str() + | ||
| ",port=" + std::to_string(peers[i].addr.port) + "\r\n"; | ||
| tmp_stream << "raft_node" << std::to_string(i) << ":addr=" << butil::ip2str(peers[i].addr.ip).c_str() | ||
| << ",port=" << std::to_string(peers[i].addr.port) << "\r\n"; | ||
| } | ||
| } | ||
|
|
||
| client->AppendString(message); | ||
| message.append(tmp_stream.str()); |
There was a problem hiding this comment.
修复 tmp_stream 设置错误消息的问题
在 InfoRaft 方法中，tmp_stream 被设置为错误消息字符串，这可能会导致问题。建议使用 message 直接追加错误消息。
if (PRAFT.IsLeader()) {
std::vector<braft::PeerId> peers;
auto status = PRAFT.GetListPeers(&peers);
if (!status.ok()) {
- tmp_stream.str("-ERR ");
- tmp_stream << status.error_str() << "\r\n";
+ message += "-ERR ";
+ message += status.error_str() + "\r\n";
return;
}
for (int i = 0; i < peers.size(); i++) {
tmp_stream << "raft_node" << std::to_string(i) << ":addr=" << butil::ip2str(peers[i].addr.ip).c_str()
<< ",port=" << std::to_string(peers[i].addr.port) << "\r\n";
}
}
message.append(tmp_stream.str());
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void InfoCmd::InfoRaft(std::string& message) { | |
| if (!PRAFT.IsInitialized()) { | |
| return client->SetRes(CmdRes::kErrOther, "Don't already cluster member"); | |
| message += "-ERR Not a cluster member.\r\n"; | |
| return; | |
| } | |
| auto node_status = PRAFT.GetNodeStatus(); | |
| if (node_status.state == braft::State::STATE_END) { | |
| return client->SetRes(CmdRes::kErrOther, "Node is not initialized"); | |
| message += "-ERR Node is not initialized.\r\n"; | |
| return; | |
| } | |
| std::string message; | |
| message += "raft_group_id:" + PRAFT.GetGroupID() + "\r\n"; | |
| message += "raft_node_id:" + PRAFT.GetNodeID() + "\r\n"; | |
| message += "raft_peer_id:" + PRAFT.GetPeerID() + "\r\n"; | |
| std::stringstream tmp_stream; | |
| tmp_stream << "raft_group_id:" << PRAFT.GetGroupID() << "\r\n"; | |
| tmp_stream << "raft_node_id:" << PRAFT.GetNodeID() << "\r\n"; | |
| tmp_stream << "raft_peer_id:" << PRAFT.GetPeerID() << "\r\n"; | |
| if (braft::is_active_state(node_status.state)) { | |
| message += "raft_state:up\r\n"; | |
| tmp_stream << "raft_state:up\r\n"; | |
| } else { | |
| message += "raft_state:down\r\n"; | |
| tmp_stream << "raft_state:down\r\n"; | |
| } | |
| message += "raft_role:" + std::string(braft::state2str(node_status.state)) + "\r\n"; | |
| message += "raft_leader_id:" + node_status.leader_id.to_string() + "\r\n"; | |
| message += "raft_current_term:" + std::to_string(node_status.term) + "\r\n"; | |
| tmp_stream << "raft_role:" << std::string(braft::state2str(node_status.state)) << "\r\n"; | |
| tmp_stream << "raft_leader_id:" << node_status.leader_id.to_string() << "\r\n"; | |
| tmp_stream << "raft_current_term:" << std::to_string(node_status.term) << "\r\n"; | |
| if (PRAFT.IsLeader()) { | |
| std::vector<braft::PeerId> peers; | |
| auto status = PRAFT.GetListPeers(&peers); | |
| if (!status.ok()) { | |
| return client->SetRes(CmdRes::kErrOther, status.error_str()); | |
| tmp_stream.str("-ERR "); | |
| tmp_stream << status.error_str() << "\r\n"; | |
| return; | |
| } | |
| for (int i = 0; i < peers.size(); i++) { | |
| message += "raft_node" + std::to_string(i) + ":addr=" + butil::ip2str(peers[i].addr.ip).c_str() + | |
| ",port=" + std::to_string(peers[i].addr.port) + "\r\n"; | |
| tmp_stream << "raft_node" << std::to_string(i) << ":addr=" << butil::ip2str(peers[i].addr.ip).c_str() | |
| << ",port=" << std::to_string(peers[i].addr.port) << "\r\n"; | |
| } | |
| } | |
| client->AppendString(message); | |
| message.append(tmp_stream.str()); | |
| void InfoCmd::InfoRaft(std::string& message) { | |
| if (!PRAFT.IsInitialized()) { | |
| message += "-ERR Not a cluster member.\r\n"; | |
| return; | |
| } | |
| auto node_status = PRAFT.GetNodeStatus(); | |
| if (node_status.state == braft::State::STATE_END) { | |
| message += "-ERR Node is not initialized.\r\n"; | |
| return; | |
| } | |
| std::stringstream tmp_stream; | |
| tmp_stream << "raft_group_id:" << PRAFT.GetGroupID() << "\r\n"; | |
| tmp_stream << "raft_node_id:" << PRAFT.GetNodeID() << "\r\n"; | |
| tmp_stream << "raft_peer_id:" << PRAFT.GetPeerID() << "\r\n"; | |
| if (braft::is_active_state(node_status.state)) { | |
| tmp_stream << "raft_state:up\r\n"; | |
| } else { | |
| tmp_stream << "raft_state:down\r\n"; | |
| } | |
| tmp_stream << "raft_role:" << std::string(braft::state2str(node_status.state)) << "\r\n"; | |
| tmp_stream << "raft_leader_id:" << node_status.leader_id.to_string() << "\r\n"; | |
| tmp_stream << "raft_current_term:" << std::to_string(node_status.term) << "\r\n"; | |
| if (PRAFT.IsLeader()) { | |
| std::vector<braft::PeerId> peers; | |
| auto status = PRAFT.GetListPeers(&peers); | |
| if (!status.ok()) { | |
| message += "-ERR "; | |
| message += status.error_str() + "\r\n"; | |
| return; | |
| } | |
| for (int i = 0; i < peers.size(); i++) { | |
| tmp_stream << "raft_node" << std::to_string(i) << ":addr=" << butil::ip2str(peers[i].addr.ip).c_str() | |
| << ",port=" << std::to_string(peers[i].addr.port) << "\r\n"; | |
| } | |
| } | |
| message.append(tmp_stream.str()); | |
| } |
There was a problem hiding this comment.
@hahahashen, great to hear that the changes have been implemented! If there's anything else you need help with, feel free to ask. 😊
Learnings used
Learnt from: hahahashen
PR: OpenAtomFoundation/pikiwidb#338
File: src/cmd_admin.cc:248-289
Timestamp: 2024-07-10T14:45:11.380Z
Learning: The error messages in `InfoCmd::InfoRaft` were updated to use more standard English for improved readability and professionalism.
src/client.h
Outdated
| CommandStatistics(const CommandStatistics& other) { | ||
| cmd_time_consuming_.store(other.cmd_time_consuming_.load()); | ||
| cmd_count_.store(other.cmd_count_.load()); | ||
| } |
There was a problem hiding this comment.
优化拷贝构造函数
建议在拷贝构造函数中使用成员初始化列表，并添加自赋值检查以提高性能和安全性。
CommandStatistics(const CommandStatistics& other)
: cmd_count_(other.cmd_count_.load()), cmd_time_consuming_(other.cmd_time_consuming_.load()) {
}There was a problem hiding this comment.
@hahahashen, great to hear that the change has been implemented. If there's anything else you need help with, feel free to ask!
(=^･^=)
hahahashen
force-pushed
the
feat/addCmdInfo
branch
from
e0b725e
to
44f20eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
src/pikiwidb.cc (1)
16-16: 建议使用std::chrono替换time.h。
std::chrono提供了更好的类型安全性和更高的精度。-#include <time.h> +#include <chrono>
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- src/client.cc (4 hunks)
- src/client.h (4 hunks)
- src/cmd_admin.cc (4 hunks)
- src/cmd_admin.h (1 hunks)
- src/cmd_table_manager.cc (1 hunks)
- src/cmd_thread_pool_worker.cc (2 hunks)
- src/pikiwidb.cc (2 hunks)
- src/pikiwidb.h (2 hunks)
Files skipped from review due to trivial changes (1)
- src/cmd_table_manager.cc
Additional context used
Learnings (3)
src/cmd_admin.h (2)
Learnt from: hahahashen PR: OpenAtomFoundation/pikiwidb#338 File: src/cmd_admin.h:0-0 Timestamp: 2024-07-11T05:23:31.134Z Learning: Avoid duplicate entries in the `sectionMap` in the `InfoCmd` class to ensure each section is uniquely mapped.Learnt from: hahahashen PR: OpenAtomFoundation/pikiwidb#338 File: src/cmd_admin.cc:369-388 Timestamp: 2024-07-10T14:58:19.561Z Learning: User hahahashen implemented the suggestion to simplify the command statistics formatting logic in `InfoCmd::InfoCommandStats` by adding a helper function `formatCommandStatLine`.src/client.cc (1)
Learnt from: hahahashen PR: OpenAtomFoundation/pikiwidb#338 File: src/client.h:33-48 Timestamp: 2024-06-14T14:09:58.236Z Learning: The `PClient` class in `src/client.h` uses `std::chrono` for handling timestamps for better type safety and clarity.src/cmd_admin.cc (1)
Learnt from: hahahashen PR: OpenAtomFoundation/pikiwidb#338 File: src/cmd_admin.cc:248-289 Timestamp: 2024-07-10T14:45:11.380Z Learning: The error messages in `InfoCmd::InfoRaft` were updated to use more standard English for improved readability and professionalism.
Additional comments not posted (19)
src/cmd_thread_pool_worker.cc (1)
55-56: 确保命令统计的线程安全性。
cmd_count_和cmd_time_consuming_使用fetch_add操作是线程安全的，但请确保其他对CommandStatistics的访问也是线程安全的。src/cmd_admin.h (1)
149-155: 检查sectionMap中的重复条目。确保
sectionMap中没有重复条目，以避免映射冲突。Verification successful
sectionMap中没有重复条目。经过验证，
sectionMap中的键是唯一的，没有重复项。Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify no duplicate entries exist in sectionMap. # Test: Search for sectionMap entries. Expect: Unique entries only. rg --type cpp 'sectionMap'Length of output: 240
Script:
#!/bin/bash # Extract keys from the sectionMap definition and check for duplicates rg --type cpp --no-filename 'sectionMap' -A 7 | grep -oP '{\K[^,]+(?=,)' | sort | uniq -dLength of output: 89
Script:
#!/bin/bash # Extract and print all keys from the sectionMap definition to ensure correct extraction rg --type cpp --no-filename 'sectionMap' -A 7 | grep -oP '{\K[^,]+(?=,)' | sortLength of output: 180
src/pikiwidb.cc (1)
178-178: 初始化时间记录已添加。在
Init()方法中记录初始化时间是一个不错的改进，便于将来进行性能监控或日志记录。src/client.h (4)
25-33: 结构体CommandStatistics的设计良好。使用
std::atomic确保线程安全是一个很好的选择。
35-59: 结构体TimeStat使用std::chrono进行时间管理。这是一个很好的设计选择，提供了更高的类型安全性和精度。
276-278: 新方法GetCommandStatMap和GetTimeStat提供了良好的功能访问。这些方法增强了对
PClient操作的监控和分析能力。
336-340: 在PClient中引入了命令统计和时间统计成员变量。这些变量的引入为性能监控提供了基础。
src/client.cc (3)
360-361: 在handlePacket方法中添加了时间戳记录。通过记录入队时间来增强命令执行时间的跟踪是一个很好的改进。
432-432: 在PClient构造函数中初始化TimeStat对象。这为时间统计功能提供了基础。
674-677: 新方法GetCommandStatMap和GetTimeStat提供了对内部状态的访问。这些方法使得外部可以更好地监控和分析
PClient的操作。src/cmd_admin.cc (9)
156-178: 改进常量命名建议使用全大写字母和下划线分隔的方式来命名静态字符串常量，以提高代码的规范性和可读性。
- const std::string InfoCmd::kInfoSection = "info"; - const std::string InfoCmd::kAllSection = "all"; - const std::string InfoCmd::kServerSection = "server"; - const std::string InfoCmd::kStatsSection = "stats"; - const std::string InfoCmd::kCPUSection = "cpu"; - const std::string InfoCmd::kDataSection = "data"; - const std::string InfoCmd::kCommandStatsSection = "commandstats"; - const std::string InfoCmd::kRaftSection = "raft"; + const std::string InfoCmd::K_INFO_SECTION = "INFO"; + const std::string InfoCmd::K_ALL_SECTION = "ALL"; + const std::string InfoCmd::K_SERVER_SECTION = "SERVER"; + const std::string InfoCmd::K_CLIENTS_SECTION = "CLIENTS"; + const std::string InfoCmd::K_STATS_SECTION = "STATS"; + const std::string InfoCmd::K_CPU_SECTION = "CPU"; + const std::string InfoCmd::K_REPLICATION_SECTION = "REPLICATION"; + const std::string InfoCmd::K_KEYSPACE_SECTION = "KEYSPACE"; + const std::string InfoCmd::K_DATA_SECTION = "DATA"; + const std::string InfoCmd::K_ROCKSDB_SECTION = "ROCKSDB"; + const std::string InfoCmd::K_DEBUG_SECTION = "DEBUG"; + const std::string InfoCmd::K_COMMAND_STATS_SECTION = "COMMANDSTATS"; + const std::string InfoCmd::K_CACHE_SECTION = "CACHE"; + const std::string InfoCmd::K_RAFT_SECTION = "RAFT";
182-205: 检查参数数量以避免潜在问题在使用
std::transform转换命令部分名称为小写之前，应该检查argv_.size()是否大于 1，以避免潜在的问题。if (argc == 1) { info_section_ = kInfo; return true; } if (argc > 1) { std::string argv_ = client->argv_[1].data(); // convert section to lowercase std::transform(argv_.begin(), argv_.end(), argv_.begin(), [](unsigned char c) { return std::tolower(c); }); if (argc == 2) { auto it = sectionMap.find(argv_); if (it != sectionMap.end()) { info_section_ = it->second; } else { client->SetRes(CmdRes::kErrOther, "the cmd is not supported"); return false; } } else { client->SetRes(CmdRes::kSyntaxErr); return false; } } return true; }
207-254: 减少代码重复
DoCmd方法中的 switch-case 结构提高了代码的可读性和可维护性，但存在一些代码重复，可以通过提取公共逻辑来减少重复。void InfoCmd::DoCmd(PClient* client) { std::string info; switch (info_section_) { case kInfo: case kInfoAll: InfoServer(info); info.append("\r\n"); InfoData(info); info.append("\r\n"); InfoStats(info); info.append("\r\n"); if (info_section_ == kInfoAll) { InfoCommandStats(client, info); info.append("\r\n"); } InfoCPU(info); info.append("\r\n"); break; case kInfoServer: InfoServer(info); break; case kInfoStats: InfoStats(info); break; case kInfoCPU: InfoCPU(info); break; case kInfoData: InfoData(info); break; case kInfoCommandStats: InfoCommandStats(client, info); break; case kInfoRaft: InfoRaft(info); break; default: break; } client->AppendString(info); }
271-312: 修复tmp_stream设置错误消息的问题在
InfoRaft方法中，tmp_stream被设置为错误消息字符串，这可能会导致问题。建议使用message直接追加错误消息。if (PRAFT.IsLeader()) { std::vector<braft::PeerId> peers; auto status = PRAFT.GetListPeers(&peers); if (!status.ok()) { - tmp_stream.str("-ERR "); - tmp_stream << status.error_str() << "\r\n"; + message += "-ERR "; + message += status.error_str() + "\r\n"; return; } for (int i = 0; i < peers.size(); i++) { tmp_stream << "raft_node" << std::to_string(i) << ":addr=" << butil::ip2str(peers[i].addr.ip).c_str() << ",port=" << std::to_string(peers[i].addr.port) << "\r\n"; } } message.append(tmp_stream.str()); }
315-342: 检查服务器信息的格式化
InfoServer方法中使用std::stringstream来构建服务器信息。确保所有必要的细节都已包含，并且格式正确。void InfoCmd::InfoServer(std::string& info) { static struct utsname host_info; static bool host_info_valid = false; if (!host_info_valid) { uname(&host_info); host_info_valid = true; } time_t current_time_s = time(nullptr); std::stringstream tmp_stream; char version[32]; snprintf(version, sizeof(version), "%s", KPIKIWIDB_VERSION); tmp_stream << "# Server\r\n"; tmp_stream << "PikiwiDB_version:" << version << "\r\n"; tmp_stream << "PikiwiDB_build_git_sha:" << KPIKIWIDB_GIT_COMMIT_ID << "\r\n"; tmp_stream << "Pikiwidb_build_compile_date: " << KPIKIWIDB_BUILD_DATE << "\r\n"; tmp_stream << "os:" << host_info.sysname << " " << host_info.release << " " << host_info.machine << "\r\n"; tmp_stream << "arch_bits:" << (reinterpret_cast<char*>(&host_info.machine) + strlen(host_info.machine) - 2) << "\r\n"; tmp_stream << "process_id:" << getpid() << "\r\n"; tmp_stream << "run_id:" << static_cast<std::string>(g_config.run_id) << "\r\n"; tmp_stream << "tcp_port:" << g_config.port << "\r\n"; tmp_stream << "uptime_in_seconds:" << (current_time_s - g_pikiwidb->Start_time_s()) << "\r\n"; tmp_stream << "uptime_in_days:" << (current_time_s / (24 * 3600) - g_pikiwidb->Start_time_s() / (24 * 3600) + 1) << "\r\n"; tmp_stream << "config_file:" << g_pikiwidb->GetConfigName() << "\r\n"; info.append(tmp_stream.str()); }
345-352: 检查统计信息的计算和展示
InfoStats方法中构建了统计信息，确保统计数据的计算和展示是正确的。void InfoCmd::InfoStats(std::string& info) { std::stringstream tmp_stream; tmp_stream << "# Stats" << "\r\n"; tmp_stream << "is_bgsaving:" << (PREPL.IsBgsaving() ? "Yes" : "No") << "\r\n"; tmp_stream << "slow_logs_count:" << PSlowLog::Instance().GetLogsCount() << "\r\n"; info.append(tmp_stream.str()); }
355-375: 检查CPU使用信息的测量和格式化
InfoCPU方法中构建了CPU使用信息，确保测量和格式化是准确的。void InfoCmd::InfoCPU(std::string& info) { struct rusage self_ru; struct rusage c_ru; getrusage(RUSAGE_SELF, &self_ru); getrusage(RUSAGE_CHILDREN, &c_ru); std::stringstream tmp_stream; tmp_stream << "# CPU" << "\r\n"; tmp_stream << "used_cpu_sys:" << std::setiosflags(std::ios::fixed) << std::setprecision(2) << static_cast<float>(self_ru.ru_stime.tv_sec) + static_cast<float>(self_ru.ru_stime.tv_usec) / 1000000 << "\r\n"; tmp_stream << "used_cpu_user:" << std::setiosflags(std::ios::fixed) << std::setprecision(2) << static_cast<float>(self_ru.ru_utime.tv_sec) + static_cast<float>(self_ru.ru_utime.tv_usec) / 1000000 << "\r\n"; tmp_stream << "used_cpu_sys_children:" << std::setiosflags(std::ios::fixed) << std::setprecision(2) << static_cast<float>(c_ru.ru_stime.tv_sec) + static_cast<float>(c_ru.ru_stime.tv_usec) / 1000000 << "\r\n"; tmp_stream << "used_cpu_user_children:" << std::setiosflags(std::ios::fixed) << std::setprecision(2) << static_cast<float>(c_ru.ru_utime.tv_sec) + static_cast<float>(c_ru.ru_utime.tv_usec) / 1000000 << "\r\n"; info.append(tmp_stream.str()); }
378-382: 检查数据库信息的正确性和格式化
InfoData方法中追加了数据库信息，确保信息的正确性和格式化。void InfoCmd::InfoData(std::string& message) { message += DATABASES_NUM + std::string(":") + std::to_string(pikiwidb::g_config.databases) + "\r\n"; message += ROCKSDB_NUM + std::string(":") + std::to_string(pikiwidb::g_config.db_instance_num) + "\r\n"; message += ROCKSDB_VERSION + std::string(":") + ROCKSDB_NAMESPACE::GetRocksVersionAsString() + "\r\n"; }
392-405: 检查命令统计信息的计算和格式化
InfoCommandStats方法中构建了命令统计信息，确保计算和格式化是准确的。void InfoCmd::InfoCommandStats(PClient* client, std::string& info) { std::stringstream tmp_stream; tmp_stream.precision(2); tmp_stream.setf(std::ios::fixed); tmp_stream << "# Commandstats" << "\r\n"; auto cmdstat_map = client->GetCommandStatMap(); for (auto iter : *cmdstat_map) { if (iter.second.cmd_count_ != 0) { tmp_stream << iter.first << ":" << FormatCommandStatLine(iter.second); } } info.append(tmp_stream.str()); }
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/client.h (4 hunks)
- src/cmd_admin.cc (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/client.h
- src/cmd_admin.cc
| InfoRaft(info); | ||
| break; | ||
| default: | ||
| break; |
There was a problem hiding this comment.
有个吹毛求疵的建议是，把 switch 换成 map，每个 kXxx 字符串对应一个函数，方便扩展
| tmp_stream << "run_id:" << static_cast<std::string>(g_config.run_id) << "\r\n"; | ||
| tmp_stream << "tcp_port:" << g_config.port << "\r\n"; | ||
| tmp_stream << "uptime_in_seconds:" << (current_time_s - g_pikiwidb->Start_time_s()) << "\r\n"; | ||
| tmp_stream << "uptime_in_days:" << (current_time_s / (24 * 3600) - g_pikiwidb->Start_time_s() / (24 * 3600) + 1) |
There was a problem hiding this comment.
这个 Start_time_s 命名需要统一下： StartTimeS
| << "\r\n"; | ||
|
|
||
| tmp_stream << "is_bgsaving:" << (PREPL.IsBgsaving() ? "Yes" : "No") << "\r\n"; | ||
| tmp_stream << "slow_logs_count:" << PSlowLog::Instance().GetLogsCount() << "\r\n"; |
There was a problem hiding this comment.
我个人还是觉得，这种固定长度的形式，还是采用 sprintf 和 fmt::format ，把它一口气生成一个大的字符串（与一口气生成相对的是，用后者这种格式生成字符串的形式，还要分开生成然后再组合），性能会好一点
src/client.h
Outdated
| void SetDequeueTs(TimePoint now_time) { dequeue_ts_ = now_time; } | ||
| void SetProcessDoneTs(TimePoint now_time) { process_done_ts_ = now_time; } | ||
|
|
||
| TimePoint enqueue_ts_; |
There was a problem hiding this comment.
这里是不是可以初始化为 TimePoint::min()?
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/client.h (4 hunks)
Additional comments not posted (6)
src/client.h (6)
25-32: 新增结构体CommandStatistics该结构体使用原子计数器来跟踪命令执行的统计数据，设计合理。
代码变更已批准。
34-58: 新增结构体TimeStat该结构体使用
std::chrono::time_point来测量和管理时间统计数据，设计合理。代码变更已批准。
276-276: 新增方法GetCommandStatMap该方法返回指向命令统计数据映射的指针，设计合理。
代码变更已批准。
277-277: 新增方法GetTimeStat该方法返回指向
TimeStat实例的共享指针，设计合理。代码变更已批准。
338-338: 新增成员变量cmdstat_map_该成员变量用于存储命令统计数据，设计合理。
代码变更已批准。
339-339: 新增成员变量time_stat_该成员变量用于存储时间统计数据，设计合理。
代码变更已批准。
误删除了之前那个PR的分支，所以提了一个新的PR，原来PR comment可看 #326 ，都进行了回复与处理
Summary by CodeRabbit
新功能
InfoSection枚举，支持不同信息区域的映射和处理。改进
PClient类，增加了获取命令统计信息和时间统计的功能。PikiwiDB类中添加了开始时间的初始化和检索方法。修复
InfoCmd类的方法以处理不同类型的命令和信息。DoCmd方法的逻辑，提供更详细的错误处理和信息格式。重构