Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added command time statistics #1660

Merged
merged 3 commits into from
Jul 4, 2023
Merged

Added command time statistics #1660

merged 3 commits into from
Jul 4, 2023

Conversation

Mixficsol
Copy link
Collaborator

fix #977

include/pika_admin.h Show resolved Hide resolved
@@ -20,6 +26,8 @@ class PikaCmdTableManager {
uint32_t DistributeKey(const std::string& key, uint32_t slot_num);

private:
clock_t start_, end_;
int cost_;
std::shared_ptr<Cmd> NewCommand(const std::string& opt);

void InsertCurrentThreadDistributionMap();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some observations and suggestions based on the code patch you provided:

  1. In the code patch, a new struct pikaCommandStatistics is defined to store command statistics. It would be helpful to clarify whether this struct is being used appropriately in the code and provides the desired functionality.

  2. In the PikaCmdTableManager class, three new variables are introduced: start_, end_, and cost_. The purpose of these variables is not clear from the provided code patch. It's recommended to add comments or documentation to explain their usage.

  3. It's unclear what the purpose of the NewCommand function is since it's not implemented in the provided code. If this function is intended to create a new command object, make sure it is implemented correctly and used appropriately in the code.

  4. Code review often requires examining the code in its entirety, including the implementation details of functions and their interactions. Without the complete code, it's challenging to provide a thorough review and identify potential bugs or improvements accurately.

  5. It's essential to perform comprehensive testing to ensure the correctness and efficiency of the modified code. This includes unit tests, integration tests, and checking for any potential performance bottlenecks.

Remember, conducting a detailed code review typically requires reviewing the entire codebase and understanding the specific requirements and constraints of the project. The provided code patch does not provide sufficient information for an in-depth evaluation.

include/pika_admin.h Show resolved Hide resolved
@@ -20,6 +26,9 @@ class PikaCmdTableManager {
uint32_t DistributeKey(const std::string& key, uint32_t slot_num);

private:
clock_t start_, end_;
int cost_;
std::mutex mtx_;
std::shared_ptr<Cmd> NewCommand(const std::string& opt);

void InsertCurrentThreadDistributionMap();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some observations and suggestions for the code patch you provided:

  1. In the PikaCmdTableManager class, there is a new member variable pikaCommandStatistics, but it is not used or referenced anywhere in the code. If it's intended to be used later or in future modifications, it's fine; otherwise, you can remove it since it doesn't serve any purpose currently.

  2. The variables start_, end_, cost_, and mtx_ have been added as private members of the PikaCmdTableManager class. However, their usage is not clear from the provided code snippet. Make sure they are used correctly and consistently throughout the implementation.

  3. It would be helpful to add comments/documentation within the code to explain the purpose and functionality of different methods and classes. This will improve code readability and make it easier for others (including yourself) to understand the code in the future.

  4. Consider using more descriptive variable names to improve code readability. Names like opt, cmd_count, and cmd_time_consuming can be made more meaningful to convey their purpose.

  5. Perform thorough testing of the code patch to identify any potential bugs or issues. Automated tests can help ensure that the modified code works as expected and doesn't introduce regressions.

  6. If possible, follow consistent coding conventions across the entire codebase. Ensure that indentation, naming conventions, and formatting are uniform, making the code easier to understand and maintain.

Remember that without having access to the complete codebase and understanding its context, it's challenging to provide an exhaustive review.

@@ -12,6 +12,12 @@
#include "include/pika_command.h"
#include "include/pika_data_distribution.h"

typedef struct pikaCommandStatistics {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C++中,可以去掉typedef和下面那个重复的别名,用的时候一样不用在类型前面加struct

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

好的,我会改进

#include "pstd/include/rsync.h"

using pstd::Status;

extern PikaServer* g_pika_server;
extern std::unique_ptr<PikaReplicaManager> g_pika_rm;
extern std::unordered_map<std::string, struct pikaCommandStatistics> cmdstat_map;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

可以考虑能不能改成非全局变量(比如移到dispacher下作为成员,用g_pika_server一样能访问到),如果只能作为全局变量的话,要加上g_前缀

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dispacher这个能细说一下是哪个类里面的成员吗,这个我尝试着改改

Copy link
Collaborator

@cheniujh cheniujh Jun 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/src/net/src/dispach_thread.h 这个类,它本身也是g_pika_server这个对象的成员,至于为什么不放g_pika_server中,是因为对于net包来说g_pika_server不可见,但是放在dispatch的话,就没有这个可见性的问题。

@@ -25,7 +31,13 @@ std::shared_ptr<Cmd> PikaCmdTableManager::GetCmd(const std::string& opt) {
}

std::shared_ptr<Cmd> PikaCmdTableManager::NewCommand(const std::string& opt) {
std::lock_guard<std::mutex> lock(mtx_);
start_ = clock();
Cmd* cmd = GetCmdFromDB(opt, *cmds_);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果我没有理解错,Cmd* cmd = GetCmdFromDB(opt, *cmds_);这一行只是对cmd_table做了一个find动作,来取得需要执行的命令,统计它的耗时应该不等于统计命令实际执行的耗时吧? 还是我理解错了?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个我看一下慢查询那边的统计步骤

@@ -20,6 +26,9 @@ class PikaCmdTableManager {
uint32_t DistributeKey(const std::string& key, uint32_t slot_num);

private:
clock_t start_, end_;
int cost_;
std::mutex mtx_;
Copy link
Collaborator

@cheniujh cheniujh Jun 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这把锁太大了,建议把这把锁去掉(针对这个全局的map,初始化完毕后, 后面只有find,没有别的写操作,所以map不用保护,以结构体为单位加锁就好),可以把结构体pikaCommandStatistics改成:

struct PikaCommandStatistics {
std::string cmd_name;
std::atomic<int32_t> cmd_count;
std::atomic<int32_t> cmd_time_consuming; //存储总耗时的毫秒数
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我刚开始也是这样想的,但是刚开始对map有个初始化操作,如果用原子变量的话,是不是不能进行赋值操作

PikaCmdTableManager::PikaCmdTableManager() {
cmds_ = std::make_unique<CmdTable>();
cmds_->reserve(300);
InitCmdTable(cmds_.get());
for (const auto& cmd : *cmds_) {
pikaCommandStatistics Cmd = {cmd.first, 0, 0};
cmdstat_map.emplace(Cmd.cmd_name, Cmd);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cheniujh 我刚开始也是这样想的,但是刚开始对map有个初始化操作,如果用原子变量的话,是不是不能进行赋值操作

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

当然可以赋值,如果不能用{cmd.first, 0, 0};这样的形式,可以改成这样:
1.定义的地方直接给初值:
struct PikaCommandStatistics {
std::string cmd_name;
std::atomic<int32_t> cmd_count{0};
std::atomic<int32_t> cmd_time_consuming{0}; //存储总耗时的毫秒数
};
2.后面只给cmd_name赋值即可:
pikaCommandStatistics cmd;
cmd.cmd_name = cmd.first;

另外提个建议:变量名不要Cmd这种大写开头,如果直接改成cmd_stat比较合适

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

明白,这个变量名指的是改成pikaCommandStatistics cmd_stat这样吗;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

嗯嗯

std::atomic<int32_t> cmd_count{0};
std::atomic<int32_t> cmd_time_consuming {0};
};

class PikaCmdTableManager {
public:
PikaCmdTableManager();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the code patch introduces a new structure called pikaCommandStatistics and adds it to the PikaCmdTableManager class. Here are some observations and suggestions for improvement:

  1. The pikaCommandStatistics structure seems reasonable, encapsulating the name of a command, its count, and time-consuming properties. However, consider making the member variables private and providing getter methods for accessing them.

  2. The current implementation uses std::atomic<int32_t> for counting and tracking time-consuming operations. This is appropriate if multiple threads access and modify these variables concurrently. Ensure that thread safety is required before using atomic types.

  3. Consider adding appropriate documentation and comments to clarify the purpose and usage of the new code.

  4. It would be helpful to see the rest of the code, as the provided snippet lacks enough context to perform an in-depth review. Feel free to provide additional relevant sections if you'd like a more comprehensive evaluation.

Please note that without seeing the entire codebase and understanding the project requirements, it is difficult to identify specific bug risks or suggest further improvements.

src/pika_admin.cc Show resolved Hide resolved
@@ -132,6 +138,8 @@ std::shared_ptr<Cmd> PikaClientConn::DoCmd(const PikaCmdArgsType& argv, const st
void PikaClientConn::ProcessSlowlog(const PikaCmdArgsType& argv, uint64_t start_us, uint64_t do_duration) {
int32_t start_time = start_us / 1000000;
int64_t duration = pstd::NowMicros() - start_us;


if (duration > g_pika_conf->slowlog_slower_than()) {
g_pika_server->SlowlogPushEntry(argv, start_time, duration);
if (g_pika_conf->slowlog_write_errorlog()) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Error handling: It's a good practice to handle potential errors. In the code snippet, the line iter->second.cmd_count++; assumes that opt exists in the g_cmdstat_map, but it doesn't check if the element is present. It would be safer to use find to check if the element exists before accessing it.

  2. Naming conventions: Follow consistent naming conventions for variables and functions to enhance code readability. For example, instead of using single-character names like c_ptr, consider more descriptive names.

  3. Code organization: Make sure related code is grouped together logically. In the given patch, there is an empty line between the declaration of the variable duration and the following if statement in ProcessSlowlog. Removing the extra empty line would improve code organization.

  4. Performance optimization: Assess whether the usage of an unordered map (g_cmdstat_map) is the most efficient data structure for your needs. Depending on the size of the map and the operations performed on it, other data structures like std::map or even custom implementations might provide better performance.

Remember to thoroughly test the changes after implementing these suggestions to ensure the code functions as intended.

std::atomic<int32_t> cmd_count{0};
std::atomic<int32_t> cmd_time_consuming {0};
};

class PikaCmdTableManager {
public:
PikaCmdTableManager();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch you provided appears to introduce a new class called PikaCmdTableManager and a new struct called pikaCommandStatistics. Here are some suggestions for code review:

  1. Ensure header inclusion: Make sure that all necessary headers are included in the source file where this code patch is applied. Check if include/pika_command.h and include/pika_data_distribution.h are indeed required for the functionality of PikaCmdTableManager.

  2. Naming conventions: In C++, it's generally recommended to follow a consistent naming convention. The struct name pikaCommandStatistics violates the common convention of starting type names with an uppercase letter. Consider renaming it to PikaCommandStatistics for better adherence to conventions.

  3. Initialization of atomic members: You have used initializer lists to initialize the cmd_count and cmd_time_consuming atomic members of pikaCommandStatistics. This is a good practice for initializing atomic variables correctly.

  4. Thread safety: Since cmd_count and cmd_time_consuming are atomic variables, it appears that you're intending to use them concurrently from multiple threads. Ensure that appropriate synchronization mechanisms are implemented when accessing or modifying these variables to avoid data races and ensure correctness.

  5. Code organization: Please verify that the new code fits well within the existing codebase. Pay attention to the class layout and its interactions with other components to ensure clarity and maintainability.

  6. Error handling: Assess whether error handling mechanisms are implemented appropriately within the new class and struct. Consider adding error handling logic where necessary to improve robustness.

  7. Testing: After the code patch is applied, perform extensive testing to validate the behavior of the new functionality and ensure it meets your requirements. Include both positive and negative test cases to cover various scenarios.

Remember, a more detailed review can be provided with access to the complete codebase and specific requirements or context for the code.

}
info.append(tmp_stream.str());
}

void ConfigCmd::DoInitial() {
if (!CheckArg(argv_.size())) {
res_.SetRes(CmdRes::kWrongNum, kCmdNameConfig);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the code patch seems to be adding functionality related to command statistics in the existing codebase. Here are some suggestions and improvements:

  1. It's generally a good practice to include relevant headers within the implementation file rather than relying on external includes through other files. Ensure that all necessary headers are included directly in the source file InfoCmd.cc.

  2. When using external variables like g_pika_server, g_pika_rm, and g_cmdstat_map, it is recommended to use forward declarations in header files instead of including the actual headers. This helps reduce unnecessary dependencies and compilation times.

  3. Consider encapsulating the global g_cmdstat_map within a class or namespace to avoid potential name clashes and improve code organization.

  4. The usage of std::stringstream in InfoCommandstats could be replaced with std::string concatenation for simpler code. For example, you can change:

tmp_stream << "cmdstat_" << iter->second.cmd_name << ":"
   << "calls=" << iter->second.cmd_count << ",usec="
   << iter->second.cmd_time_consuming
   << ",usec_per_call=" << (iter->second.cmd_time_consuming * 1.0) / iter->second.cmd_count << "\r\n";

to

tmp_stream += "cmdstat_" + iter->second.cmd_name + ":";
tmp_stream += "calls=" + std::to_string(iter->second.cmd_count) + ",usec=";
tmp_stream += std::to_string(iter->second.cmd_time_consuming);
tmp_stream += ",usec_per_call=";
tmp_stream += std::to_string((iter->second.cmd_time_consuming * 1.0) / iter->second.cmd_count) + "\r\n";
  1. Make sure proper error handling and input validation are performed in the code, especially when processing argv_ and accessing elements within it. Ensure that all possible edge cases are properly handled and invalid input does not lead to unexpected behavior or crashes.

  2. Consider adding comments to clearly explain the purpose and functionality of each function and section of the code to improve code readability and maintainability.

  3. Perform thorough testing of the modified functionality to ensure the correct behavior of the added command statistics feature.

These suggestions should help improve the code's quality and reduce potential bugs. However, without a complete understanding of the entire codebase and its requirements, it's difficult to identify all possible issues or improvements.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chatgpt说的对

std::atomic<int32_t> cmd_count {0};
std::atomic<int32_t> cmd_time_consuming {0};
};

class PikaCmdTableManager {
public:
PikaCmdTableManager();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The provided code patch introduces a new struct pikaCommandStatistics and makes changes to the class PikaCmdTableManager. Here's a brief code review:

  1. The pikaCommandStatistics struct looks fine. It has a constructor that initializes the cmd_name, and two atomic counters (cmd_count and cmd_time_consuming) to track command statistics.

  2. In the PikaCmdTableManager class, there's no visible constructor definition in the code snippet you provided. Make sure it is implemented properly.

  3. Consider adding access modifiers (public, private, or protected) explicitly to the member functions and variables in both pikaCommandStatistics and PikaCmdTableManager classes for clarity.

  4. It's generally a good practice to follow consistent naming conventions. Ensure that the naming convention used for the structures, classes, variables, and functions aligns with the existing codebase.

  5. To handle concurrent updates safely, ensure that proper synchronization mechanisms (like locks or atomic operations) are used when accessing and modifying shared data, such as the atomic counters in pikaCommandStatistics.

  6. Since this is a code snippet, further analysis would require more context about how these classes are used and where this code is being called. It's essential to consider the broader context of the application to identify any potential bug risks or improvement suggestions accurately.

Remember to thoroughly test the modified code and consider additional tests for any new functionality introduced to reduce the risk of bugs.

src/pika_admin.cc Show resolved Hide resolved
src/pika_client_conn.cc Show resolved Hide resolved
src/pika_cmd_table_manager.cc Outdated Show resolved Hide resolved
std::string cmd_name;
std::atomic<int32_t> cmd_count {0};
std::atomic<int32_t> cmd_time_consuming {0};
} pikaCommandStatistics;
using CmdTable = std::unordered_map<std::string, std::unique_ptr<Cmd>>;

// Method for Cmd Table
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the code patch adds a new struct called pikaCommandStatistics and modifies the Cmd class and CmdTable type. Here are some observations and suggestions:

  1. In the code patch, the pikaCommandStatistics struct is defined twice. You should remove one of the definitions to avoid duplicate symbol errors.

  2. It might be a good idea to use uppercase letters for struct names, following common naming conventions. For example, consider using PikaCommandStatistics instead of pikaCommandStatistics.

  3. The cmd_name member in the pikaCommandStatistics struct is redundant because it duplicates the name already present in the Cmd class. You can remove the cmd_name member from the pikaCommandStatistics struct and use name_ from the Cmd class when needed.

  4. Consider initializing the atomic variables cmd_count and cmd_time_consuming within the constructor initializer list rather than directly within the struct definition. This allows you to set their initial values explicitly, if necessary.

  5. When defining CmdTable as an unordered map, it's generally better to use smart pointers (std::unique_ptr) instead of raw pointers to manage memory and prevent memory leaks. Therefore, updating the CmdTable type to use std::unique_ptr<Cmd> is recommended.

  6. It would be helpful to see the rest of the code or a larger portion of it in order to provide more thorough feedback. Without additional context, it's difficult to identify any potential bug risks or suggest specific improvements.

Remember to thoroughly test your code after making these changes to ensure its correctness and performance.

}
info.append(tmp_stream.str());
}

void ConfigCmd::DoInitial() {
if (!CheckArg(argv_.size())) {
res_.SetRes(CmdRes::kWrongNum, kCmdNameConfig);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some observations and suggestions for the code patch:

  1. Include Order: Ensure that the included headers are in the correct order.

  2. Variable Naming: Use more descriptive variable names to improve code readability. For example, iter can be renamed to something like command_stat_iterator.

  3. Commandstats Section: The implementation of the InfoCommandstats function is incomplete. It references a CmdTable, but it doesn't seem to be initialized or populated with data. Make sure that the cmdstat_map is properly initialized before iterating over it.

  4. Omitted Code: Depending on the omitted code, it's possible that there are other functions or logic missing that affect the overall behavior of the program. Please ensure that all necessary code is present for a complete understanding of the program flow.

  5. Error Handling: It's important to handle any potential errors that may arise during the execution of the code. Make sure to add appropriate error handling and consider edge cases, such as when an invalid info section is provided.

  6. Commenting: Consider adding comments above functions or sections of code to provide a brief description of their purpose or functionality. This can help other developers understand the code more easily.

Remember to thoroughly test the code after making improvements or changes to ensure its correctness and reliability.

src/pika_admin.cc Show resolved Hide resolved
src/pika_admin.cc Show resolved Hide resolved
tmp_stream.precision(2);
tmp_stream.setf(std::ios::fixed);
tmp_stream << "# Commandstats" << "\r\n";
CmdTable cmdstat_map;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

你这么写是空的。得拿全局变量

}
info.append(tmp_stream.str());
}

void ConfigCmd::DoInitial() {
if (!CheckArg(argv_.size())) {
res_.SetRes(CmdRes::kWrongNum, kCmdNameConfig);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chatgpt说的对

@@ -118,6 +119,12 @@ std::shared_ptr<Cmd> PikaClientConn::DoCmd(const PikaCmdArgsType& argv, const st

// Process Command
c_ptr->Execute();
int64_t duration = pstd::NowMicros() - start_us;
CmdTable cmdstat_map;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个Statistics已经在cmd里了,那还find啥啊,直接用c_ptr啊

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

明白了我去改一下

include/pika_admin.h Show resolved Hide resolved
@@ -499,6 +504,7 @@ class Cmd : public std::enable_shared_from_this<Cmd> {

using CmdTable = std::unordered_map<std::string, std::unique_ptr<Cmd>>;


// Method for Cmd Table
void InitCmdTable(CmdTable* cmd_table);
Cmd* GetCmdFromDB(const std::string& opt, const CmdTable& cmd_table);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. A new struct CommandStatistics has been added to track command statistics. It includes two integer members: cmd_count and cmd_time_consuming. This addition seems reasonable for collecting statistics related to commands.

  2. The state member of type CommandStatistics has been added to the Cmd class. It's unclear how this member is being used further in the code, but it could be useful for storing and accessing command statistics within the Cmd instances.

  3. There are no apparent bug risks in the code patch. However, without the full context and details of the remaining code, it's challenging to identify potential bugs or improvements more accurately.

  4. The code patch doesn't provide any modifications to the InitCmdTable, GetCmdFromDB, or other methods, so it's not possible to review those parts for bugs or improvements.

To perform a more comprehensive code review, it would be helpful to have the complete code along with more information about the purpose and functionality of the classes and methods involved.

include/pika_admin.h Show resolved Hide resolved
@@ -118,6 +118,10 @@ std::shared_ptr<Cmd> PikaClientConn::DoCmd(const PikaCmdArgsType& argv, const st

// Process Command
c_ptr->Execute();
int64_t duration = pstd::NowMicros() - start_us;
auto iter = g_pika_cmd_table_manager->Getcmdtable();
iter->find(opt)->second->state.cmd_count++;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不是,把state封进cmd的意思就是要用c_ptr直接引用,为啥还要搜一下?

tmp_stream.setf(std::ios::fixed);
tmp_stream << "# Commandstats" << "\r\n";
auto iter = g_pika_cmd_table_manager->Getcmdtable()->begin();
while (iter != g_pika_cmd_table_manager->Getcmdtable()->end()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

用range for

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

好的,我去改改

int32_t cmd_count;
int32_t cmd_time_consuming;
};
CommandStatistics state;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CommandStatistics state;这个要塞进Cmd这个类里

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CommandStatistics state;这个要塞进Cmd这个类里

这个是在Cmd这个基类里面的,放在外面的话会访问不到

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

默认值填一下,默认0

include/pika_admin.h Show resolved Hide resolved
chejinge
chejinge previously approved these changes Jul 4, 2023
@@ -118,6 +118,10 @@ std::shared_ptr<Cmd> PikaClientConn::DoCmd(const PikaCmdArgsType& argv, const st

// Process Command
c_ptr->Execute();
int64_t duration = pstd::NowMicros() - start_us;
auto iter = g_pika_cmd_table_manager->Getcmdtable();
(*iter)[opt]->state.cmd_count++;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

感觉这里多线程访问风险还是非常大的

Copy link
Collaborator

@cheniujh cheniujh Jul 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(*iter)[opt]->state.cmd_count++;
(*iter)[opt]->state.cmd_count.fetch_add(1);

@@ -256,6 +258,7 @@ class InfoCmd : public Cmd {
void InfoData(std::string& info);
void InfoRocksDB(std::string& info);
void InfoDebug(std::string& info);
void InfoCommandstats(std::string& info);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

大小写

include/pika_admin.h Show resolved Hide resolved
chejinge
chejinge previously approved these changes Jul 4, 2023
@@ -19,6 +19,7 @@ class PikaCmdTableManager {
std::shared_ptr<Cmd> GetCmd(const std::string& opt);
uint32_t DistributeKey(const std::string& key, uint32_t slot_num);
bool CmdExist(const std::string& cmd) const;
CmdTable* Getcmdtable();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetCmdTable

@@ -210,7 +210,8 @@ class InfoCmd : public Cmd {
kInfoRocksDB,
kInfo,
kInfoAll,
kInfoDebug
kInfoDebug,
kInfoCommandstats
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kInfoCommandStats

@@ -237,6 +238,7 @@ class InfoCmd : public Cmd {
const static std::string kDataSection;
const static std::string kRocksDBSection;
const static std::string kDebugSection;
const static std::string kCommandstatsSection;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kCommandStatsSection

include/pika_admin.h Show resolved Hide resolved
@Mixficsol Mixficsol merged commit a8e343a into OpenAtomFoundation:unstable Jul 4, 2023
8 checks passed
@Mixficsol Mixficsol deleted the addcommandstatistics branch July 19, 2023 03:28
bigdaronlee163 pushed a commit to bigdaronlee163/pika that referenced this pull request Jun 8, 2024
* Optimized code format

* Add atomic variables to avoid thread collisions

* Reformat code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

redis API: support commands_duration_seconds_total
5 participants