feat:thread purge #2697
feat:thread purge #2697
Conversation
WalkthroughThis update introduces a new feature into the Pika project to manage a separate thread pool for slow commands. Key changes include adding the Changes
Poem
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 Configration 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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- conf/pika.conf (1 hunks)
- include/pika_client_processor.h (1 hunks)
- include/pika_conf.h (3 hunks)
- include/pika_server.h (4 hunks)
- src/net/src/net_thread.cc (1 hunks)
- src/pika_admin.cc (3 hunks)
- src/pika_client_processor.cc (2 hunks)
- src/pika_conf.cc (2 hunks)
- src/pika_server.cc (5 hunks)
Additional comments not posted (11)
include/pika_server.h (1)
98-98: Addition ofSetSlowCmdThreadPoolFlagmethod aligns with PR objectives.The new method
SetSlowCmdThreadPoolFlagis a good addition for managing the separation of slow and fast commands as described in the PR objectives.conf/pika.conf (1)
30-33: Addition ofslow-cmd-poolconfiguration parameter is well-documented and clear.The new configuration parameter
slow-cmd-poolis clearly documented and aligns with the PR objectives to manage the separation of slow and fast commands effectively.src/pika_conf.cc (1)
162-166: Configuration validation forslow-cmd-thread-pool-sizeis implemented correctly.This ensures that the thread pool size is within a reasonable range, preventing potential resource exhaustion.
include/pika_conf.h (3)
189-192: The getter method forslow_cmd_poolis correctly implemented with thread safety usingstd::shared_lock. This ensures that the value can be read concurrently without race conditions.
581-585: The setter method forslow_cmd_poolis correctly implemented with thread safety usingstd::lock_guard. The use ofTryPushDiffCommandsto log the change is consistent with other configuration setters in the class.
856-856: The declaration ofslow_cmd_pool_asstd::atomic<bool>is appropriate for ensuring thread safety in concurrent environments. This is consistent with the atomic usage for other similar configuration flags in the class.src/pika_server.cc (3)
46-46: Initialization ofslow_cmd_thread_pool_flag_in constructor.This change aligns with the PR's objective to manage slow command pools. The flag is correctly initialized to
false, indicating that the slow command pool is disabled by default.
104-104: Setting the slow command thread pool flag based on configuration.This line correctly sets the
slow_cmd_thread_pool_flag_based on the configuration value retrieved fromg_pika_conf->slow_cmd_pool(). This is a crucial part of integrating the new configuration parameter into the system.
204-217: Implementation ofSetSlowCmdThreadPoolFlagmethod.This method correctly handles the enabling and disabling of the slow command thread pool based on the provided flag. It also includes error handling for thread pool startup failures, which is a good practice.
src/pika_admin.cc (2)
1603-1607: Add handling for the new configuration parameterslow-cmd-pool.The implementation correctly handles the new configuration parameter
slow-cmd-poolby encoding its value as a string and appending it to the configuration body. This is consistent with the handling of other configuration parameters in the system.
Line range hint
2138-2310: Ensure proper handling of theslow-cmd-poolconfiguration in theConfigSetfunction.The
ConfigSetfunction now includes handling for theslow-cmd-poolconfiguration parameter. This change allows the system to enable or disable a pool for slow commands based on the configuration. It is important to verify that this new configuration is being handled correctly throughout the system.Verification successful
The
ConfigSetfunction is defined and used in thesrc/pika_admin.ccfile and declared in theinclude/pika_admin.hfile. To ensure that theslow-cmd-poolconfiguration is properly handled, we need to verify the implementation of theConfigSetfunction insrc/pika_admin.cc.
The previous attempt to extract the implementation of the
ConfigSetfunction did not produce any output. This might be due to a syntax issue or an incorrect pattern in theast-grepcommand. To ensure we get the correct implementation, let's refine our approach by usingrgto search for the function definition directly in thesrc/pika_admin.ccfile.
The
ConfigSetfunction implementation insrc/pika_admin.ccincludes handling for theslow-cmd-poolconfiguration. This confirms that the configuration parameter is recognized and processed within the function.
- The
slow-cmd-poolconfiguration is listed among other configuration parameters in theConfigSetfunction.Based on the gathered information, the handling of the
slow-cmd-poolconfiguration appears to be correctly integrated into theConfigSetfunction.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `slow-cmd-pool` configuration is properly handled in the `ConfigSet` function. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type python $'ConfigSet'Length of output: 59
Script:
#!/bin/bash # Description: Verify that the `slow-cmd-pool` configuration is properly handled in the `ConfigSet` function. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type cc $'ConfigSet'Length of output: 51
Script:
#!/bin/bash # Description: Verify that the `slow-cmd-pool` configuration is properly handled in the `ConfigSet` function. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type cpp $'ConfigSet'Length of output: 196
Script:
#!/bin/bash # Description: Verify the implementation of the `ConfigSet` function in `src/pika_admin.cc` to ensure proper handling of the `slow-cmd-pool` configuration. # Extract the implementation of the `ConfigSet` function from `src/pika_admin.cc`. ast-grep --lang cpp --pattern $'void ConfigCmd::ConfigSet(std::shared_ptr<DB> db) { $$$ }' src/pika_admin.ccLength of output: 111
Script:
#!/bin/bash # Description: Verify the implementation of the `ConfigSet` function in `src/pika_admin.cc` to ensure proper handling of the `slow-cmd-pool` configuration. # Extract the implementation of the `ConfigSet` function from `src/pika_admin.cc`. rg --context 20 'void ConfigCmd::ConfigSet' src/pika_admin.ccLength of output: 1382
chejinge
force-pushed
the
feat_thread_purge
branch
from
1e86940
to
a6ae2be
Compare
src/pika_server.cc
Outdated
| ret = pika_slow_cmd_thread_pool_->start_thread_pool(); | ||
| if (ret != net::kSuccess) { | ||
| dbs_.clear(); | ||
| LOG(FATAL) << "Start PikaLowLevelThreadPool Error: " << ret |
There was a problem hiding this comment.
LOG(FATAL)会导致进程abort退出，这个函数可以通过发命令的方式调用到，这个时候如果进程退出就不太好了。
There was a problem hiding this comment.
LOG(FATAL)会导致进程abort退出，这个函数可以通过发命令的方式调用到，这个时候如果进程退出就不太好了。
done
| << (ret == net::kCreateThreadError ? ": create thread error " : ": other error"); | ||
| } | ||
| } else { | ||
| pika_slow_cmd_thread_pool_->stop_thread_pool(); |
There was a problem hiding this comment.
stop_thread_pool应该是不会等队列里的任务被执行完成再结束，stop_thread_pool的时候队列里还有任务，那这些任务就被丢掉了。进而对应连接就不会被再次唤醒了。
There was a problem hiding this comment.
stop_thread_pool应该是不会等队列里的任务被执行完成再结束，stop_thread_pool的时候队列里还有任务，那这些任务就被丢掉了。进而对应连接就不会被再次唤醒了。
done
| dbs_.clear(); | ||
| LOG(FATAL) << "Start PikaLowLevelThreadPool Error: " << ret | ||
| << (ret == net::kCreateThreadError ? ": create thread error " : ": other error"); | ||
| } |
There was a problem hiding this comment.
pika_slow_cmd_thread_pool_->Schedule()的地方建议加个判断，因为现在is_slow_cmd和你开启slowcmdthreadpool是两个配置参数，如果is_slow_cmd判断为true，但是没有开slowcmdthreadpool，就会有问题。
There was a problem hiding this comment.
pika_slow_cmd_thread_pool_->Schedule()的地方建议加个判断，因为现在is_slow_cmd和你开启slowcmdthreadpool是两个配置参数，如果is_slow_cmd判断为true，但是没有开slowcmdthreadpool，就会有问题。
done
|
@CodiumAI-Agent /review |
PR Review 🔍
Code feedback:
|
|
@CodiumAI-Agent /improve |
PR Code Suggestions ✨
|
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- conf/pika.conf (1 hunks)
- include/pika_client_processor.h (1 hunks)
- include/pika_conf.h (3 hunks)
- include/pika_server.h (4 hunks)
- src/net/src/net_thread.cc (1 hunks)
- src/pika_admin.cc (3 hunks)
- src/pika_client_processor.cc (2 hunks)
- src/pika_conf.cc (2 hunks)
- src/pika_server.cc (5 hunks)
- src/rsync_server.cc (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- conf/pika.conf
- include/pika_client_processor.h
- include/pika_conf.h
- src/net/src/net_thread.cc
- src/pika_admin.cc
- src/pika_client_processor.cc
- src/pika_conf.cc
- src/pika_server.cc
Additional comments not posted (2)
include/pika_server.h (2)
98-98: The addition ofSetSlowCmdThreadPoolFlagmethod is correctly implemented.
649-649: Ensureslow_cmd_thread_pool_flag_is initialized in thePikaServerconstructor.
src/rsync_server.cc
Outdated
| @@ -33,6 +33,7 @@ void RsyncWriteResp(RsyncService::RsyncResponse& response, std::shared_ptr<net:: | |||
| RsyncServer::RsyncServer(const std::set<std::string>& ips, const int port) { | |||
| work_thread_ = std::make_unique<net::ThreadPool>(2, 100000); | |||
| rsync_server_thread_ = std::make_unique<RsyncServerThread>(ips, port, 1 * 1000, this); | |||
| rsync_server_thread_->set_thread_name("RsyncServer"); | |||
There was a problem hiding this comment.
Consider using dynamic thread names for better traceability.
- rsync_server_thread_->set_thread_name("RsyncServer");
+ rsync_server_thread_->set_thread_name("RsyncServer-" + std::to_string(port));This change appends the port number to the thread name, making it easier to identify in logs and monitoring tools.
Committable suggestion was skipped due to low confidence.
|
src/net/src/net_thread.cc
Outdated
| @@ -17,7 +17,7 @@ Thread::~Thread() = default; | |||
| void* Thread::RunThread(void* arg) { | |||
| auto thread = reinterpret_cast<Thread*>(arg); | |||
| if (!(thread->thread_name().empty())) { | |||
| SetThreadName(pthread_self(), thread->thread_name()); | |||
| SetThreadName(pthread_self(), "pika"); | |||
There was a problem hiding this comment.
1.修改ThreadPoolWorke 命名
2.修改继承thread/bgthread的命名
根据各自功能命名
There was a problem hiding this comment.
1.修改ThreadPoolWorke 命名 2.修改继承thread/bgthread的命名 根据各自功能命名
done
There was a problem hiding this comment.
Actionable comments posted: 7
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- src/net/include/backend_thread.h (1 hunks)
- src/net/include/client_thread.h (1 hunks)
- src/net/include/net_thread.h (1 hunks)
- src/net/include/server_thread.h (1 hunks)
- src/net/src/backend_thread.cc (1 hunks)
- src/net/src/client_thread.cc (1 hunks)
- src/net/src/net_thread_name.h (1 hunks)
- src/net/src/net_util.cc (1 hunks)
- src/net/src/net_util.h (1 hunks)
- src/net/src/server_thread.cc (1 hunks)
- src/net/src/thread_pool.cc (2 hunks)
- src/pika_repl_server.cc (1 hunks)
- src/pika_server.cc (9 hunks)
- src/rsync_server.cc (1 hunks)
Files skipped from review due to trivial changes (1)
- src/net/src/client_thread.cc
Files skipped from review as they are similar to previous changes (2)
- src/pika_server.cc
- src/rsync_server.cc
Additional comments not posted (4)
src/net/src/net_util.h (1)
83-83: Proper use of method overriding forset_thread_name.This ensures that the base class functionality is preserved while allowing for extension in the derived class.
src/net/src/net_util.cc (1)
129-129: Setting the thread name to "TimerTaskThread" enhances traceability and debugging. Ensure this naming convention is consistent across all threads for uniformity.Verification successful
The thread naming convention is consistent and descriptive across the system, supporting the change made in
src/net/src/net_util.cc.
tools/pika-port/pika_port_3/trysync_thread.h:TrysyncThreadsrc/pika_dispatch_thread.cc:Dispatchersrc/pika_repl_client.cc:PikaReplClientsrc/pika_repl_bgworker.cc:ReplBgWorkersrc/rsync_server.cc:RsyncServerThreadsrc/pika_repl_server.cc:PikaReplServersrc/pika_server.cc:BGSaveTaskThread,PurgelogsTaskThread,PurgeDirTaskThread,KeyScanTaskThread,Bgslotsreloadsrc/pika_cache_load_thread.cc:PikaCacheLoadThreadsrc/net/src/client_thread.cc:ClientThreadsrc/net/src/server_thread.cc:ServerThreadsrc/net/src/net_util.cc:TimerTaskThreadsrc/net/src/net_pubsub.cc:PubSubThreadsrc/net/src/dispatch_thread.cc:WorkerThreadinclude/pika_slaveping_thread.h:SlavePingThreadinclude/pika_auxiliary_thread.h:AuxiliaryThreadScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistency of thread naming across the system. # Test: Search for thread naming patterns. Expect: Consistent naming convention. rg --type cpp 'set_thread_name'Length of output: 2118
src/pika_repl_server.cc (1)
20-20: Initializing the thread pool with the name "PikaReplServer" enhances manageability and traceability. Ensure that this naming convention does not conflict with other named resources in the system.Verification successful
The naming of the thread pool as "PikaReplServer" is consistent with other components and does not cause any conflicts.
src/pika_repl_server.cc: The thread pool is named "PikaReplServer".include/pika_repl_server_thread.h: The thread name is set to "PikaReplServer".Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of the "PikaReplServer" thread pool with the rest of the system. # Test: Search for the usage of "PikaReplServer" in thread-related configurations. Expect: No conflicts or redundant definitions. rg --type cpp 'PikaReplServer'Length of output: 5685
src/net/include/server_thread.h (1)
153-153: The override ofset_thread_nameensures that server threads can have specific names, which is crucial for debugging and monitoring. Consider adding a comment explaining why this override is necessary, especially if the behavior differs from the base class.
| @@ -110,6 +110,7 @@ class BackendThread : public Thread { | |||
| */ | |||
| int StartThread() override; | |||
| int StopThread() override; | |||
| void set_thread_name(const std::string& name) override { Thread::set_thread_name(name); } | |||
There was a problem hiding this comment.
Properly overriding the set_thread_name method in BackendThread allows for dynamic and specific thread naming, which is crucial for debugging. Consider encapsulating thread naming logic within the Thread class to avoid redundancy across derived classes.
- void set_thread_name(const std::string& name) override { Thread::set_thread_name(name); }
+ // This method is now handled in the base class to avoid redundancy.Committable suggestion was skipped due to low confidence.
| @@ -110,6 +110,7 @@ class ClientThread : public Thread { | |||
| */ | |||
| int StartThread() override; | |||
| int StopThread() override; | |||
| void set_thread_name(const std::string& name) override { Thread::set_thread_name(name); } | |||
There was a problem hiding this comment.
Ensuring that ClientThread can have its thread name set dynamically aids in monitoring and debugging. As with BackendThread, consider centralizing thread naming logic to reduce redundancy.
- void set_thread_name(const std::string& name) override { Thread::set_thread_name(name); }
+ // This method is now handled in the base class to avoid redundancy.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 set_thread_name(const std::string& name) override { Thread::set_thread_name(name); } | |
| // This method is now handled in the base class to avoid redundancy. |
src/net/src/server_thread.cc
Outdated
| @@ -123,6 +123,7 @@ int ServerThread::StartThread() { | |||
| if (ret != kSuccess) { | |||
| return ret; | |||
| } | |||
| set_thread_name("ServerThread"); | |||
There was a problem hiding this comment.
Setting the thread name to "ServerThread" is hardcoded here, which could limit flexibility and clarity in debugging. Consider using a dynamic naming strategy that incorporates unique identifiers or conditions to enhance traceability.
- set_thread_name("ServerThread");
+ set_thread_name("ServerThread-" + std::to_string(port_));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.
| set_thread_name("ServerThread"); | |
| set_thread_name("ServerThread-" + std::to_string(port_)); |
| @@ -48,6 +48,8 @@ int BackendThread::StartThread() { | |||
| if (res) { | |||
| return res; | |||
| } | |||
| set_thread_name("BackendThread"); | |||
There was a problem hiding this comment.
Similar to the server thread, setting the thread name to "BackendThread" is hardcoded. Consider using a dynamic naming strategy here as well to improve monitoring and debugging capabilities.
- set_thread_name("BackendThread");
+ set_thread_name("BackendThread-" + std::to_string(port_));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.
| set_thread_name("BackendThread"); | |
| set_thread_name("BackendThread-" + std::to_string(port_)); |
| @@ -34,7 +34,7 @@ class Thread : public pstd::noncopyable { | |||
|
|
|||
| std::string thread_name() const { return thread_name_; } | |||
|
|
|||
| void set_thread_name(const std::string& name) { thread_name_ = name; } | |||
| virtual void set_thread_name(const std::string& name) { thread_name_ = name; } | |||
There was a problem hiding this comment.
Consider adding synchronization to set_thread_name.
To ensure thread safety when multiple threads might be setting the thread name simultaneously, consider using a mutex:
+ std::lock_guard<std::mutex> lock(name_mutex_);
thread_name_ = name;Add a private member std::mutex name_mutex_; to the Thread class to support this.
Committable suggestion was skipped due to low confidence.
| std::string thread_id_str = std::to_string(reinterpret_cast<unsigned long>(thread_id_)); | ||
| SetThreadName(thread_id_, thread_pool_->thread_pool_name() + "_Worker_" + thread_id_str); |
There was a problem hiding this comment.
Consider improving portability of thread ID conversion.
The current method of casting pthread_t to unsigned long may not be portable across different platforms where pthread_t is not an integer type. Consider using a portable method to obtain a string representation of pthread_t:
- std::string thread_id_str = std::to_string(reinterpret_cast<unsigned long>(thread_id_));
+ std::string thread_id_str = std::to_string(std::hash<std::thread::id>()(std::thread::id(thread_id_)));This uses the standard library's hash function to convert the thread ID to a string in a more portable manner.
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.
| std::string thread_id_str = std::to_string(reinterpret_cast<unsigned long>(thread_id_)); | |
| SetThreadName(thread_id_, thread_pool_->thread_pool_name() + "_Worker_" + thread_id_str); | |
| std::string thread_id_str = std::to_string(std::hash<std::thread::id>()(std::thread::id(thread_id_))); | |
| SetThreadName(thread_id_, thread_pool_->thread_pool_name() + "_Worker_" + thread_id_str); |
chejinge
force-pushed
the
feat_thread_purge
branch
from
58df896
to
9fe526d
Compare
chejinge
force-pushed
the
feat_thread_purge
branch
from
267179d
to
38e4c68
Compare
chejinge
force-pushed
the
feat_thread_purge
branch
from
38e4c68
to
bc329ba
Compare
chejinge
force-pushed
the
feat_thread_purge
branch
from
bc329ba
to
013f51a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (19)
- include/pika_repl_bgworker.h (1 hunks)
- src/net/include/backend_thread.h (1 hunks)
- src/net/include/client_thread.h (1 hunks)
- src/net/include/net_thread.h (1 hunks)
- src/net/include/server_thread.h (1 hunks)
- src/net/src/backend_thread.cc (1 hunks)
- src/net/src/client_thread.cc (1 hunks)
- src/net/src/dispatch_thread.cc (1 hunks)
- src/net/src/holy_thread.cc (1 hunks)
- src/net/src/holy_thread.h (1 hunks)
- src/net/src/net_thread_name.h (1 hunks)
- src/net/src/net_util.cc (1 hunks)
- src/net/src/net_util.h (1 hunks)
- src/net/src/thread_pool.cc (2 hunks)
- src/pika_repl_client.cc (1 hunks)
- src/pika_repl_server.cc (2 hunks)
- src/pika_server.cc (13 hunks)
- src/rsync_client.cc (1 hunks)
- src/rsync_server.cc (2 hunks)
Files skipped from review as they are similar to previous changes (17)
- src/net/include/backend_thread.h
- src/net/include/client_thread.h
- src/net/include/net_thread.h
- src/net/include/server_thread.h
- src/net/src/backend_thread.cc
- src/net/src/client_thread.cc
- src/net/src/holy_thread.cc
- src/net/src/holy_thread.h
- src/net/src/net_thread_name.h
- src/net/src/net_util.cc
- src/net/src/net_util.h
- src/net/src/thread_pool.cc
- src/pika_repl_client.cc
- src/pika_repl_server.cc
- src/pika_server.cc
- src/rsync_client.cc
- src/rsync_server.cc
include/pika_repl_bgworker.h
Outdated
| void SetThreadName(std::string& thread_name) { | ||
| bg_thread_.set_thread_name(thread_name); | ||
| } |
There was a problem hiding this comment.
Consider changing the parameter of SetThreadName to a const reference.
- void SetThreadName(std::string& thread_name) {
+ void SetThreadName(const std::string& thread_name) {
bg_thread_.set_thread_name(thread_name);
}This change ensures that the input string is not modified by the method, aligning with common C++ best practices for setter functions.
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 SetThreadName(std::string& thread_name) { | |
| bg_thread_.set_thread_name(thread_name); | |
| } | |
| void SetThreadName(const std::string& thread_name) { | |
| bg_thread_.set_thread_name(thread_name); | |
| } |
| @@ -66,7 +66,7 @@ int DispatchThread::StartThread() { | |||
| // Adding timer tasks and run timertaskThread | |||
| timerTaskThread_.AddTimerTask("blrpop_blocking_info_scan", 250, true, | |||
| [this] { this->ScanExpiredBlockedConnsOfBlrpop(); }); | |||
|
|
|||
| timerTaskThread_.set_thread_name("TimerTaskThread"); | |||
There was a problem hiding this comment.
Setting the thread name before starting the thread is a good practice.
Consider allowing dynamic thread naming for greater flexibility, especially useful in environments with multiple timer tasks.
chejinge
force-pushed
the
feat_thread_purge
branch
from
013f51a
to
526affd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (19)
- include/pika_repl_bgworker.h (1 hunks)
- src/net/include/backend_thread.h (1 hunks)
- src/net/include/client_thread.h (1 hunks)
- src/net/include/net_thread.h (1 hunks)
- src/net/include/server_thread.h (1 hunks)
- src/net/src/backend_thread.cc (1 hunks)
- src/net/src/client_thread.cc (1 hunks)
- src/net/src/dispatch_thread.cc (1 hunks)
- src/net/src/holy_thread.cc (1 hunks)
- src/net/src/holy_thread.h (1 hunks)
- src/net/src/net_thread_name.h (1 hunks)
- src/net/src/net_util.cc (1 hunks)
- src/net/src/net_util.h (1 hunks)
- src/net/src/thread_pool.cc (2 hunks)
- src/pika_repl_client.cc (1 hunks)
- src/pika_repl_server.cc (2 hunks)
- src/pika_server.cc (13 hunks)
- src/rsync_client.cc (1 hunks)
- src/rsync_server.cc (2 hunks)
Files skipped from review as they are similar to previous changes (18)
- include/pika_repl_bgworker.h
- src/net/include/backend_thread.h
- src/net/include/client_thread.h
- src/net/include/net_thread.h
- src/net/include/server_thread.h
- src/net/src/backend_thread.cc
- src/net/src/client_thread.cc
- src/net/src/dispatch_thread.cc
- src/net/src/holy_thread.cc
- src/net/src/holy_thread.h
- src/net/src/net_thread_name.h
- src/net/src/net_util.cc
- src/net/src/net_util.h
- src/net/src/thread_pool.cc
- src/pika_repl_client.cc
- src/pika_repl_server.cc
- src/rsync_client.cc
- src/rsync_server.cc
Additional comments not posted (1)
src/pika_server.cc (1)
104-104: EnsureSetSlowCmdThreadPoolFlagis correctly initializing the slow command thread pool based on configuration.Verification successful
The
SetSlowCmdThreadPoolFlagfunction is used in two places:
- In the
PikaServerconstructor to initialize the slow command thread pool based on the configuration.- In
pika_admin.ccto dynamically set the slow command thread pool flag based on user input.The function
SetSlowCmdThreadPoolFlagitself sets theslow_cmd_thread_pool_flag_and starts the thread pool if the flag is true. This indicates that the function is correctly designed to initialize the thread pool based on the configuration.Given this context, the placement of
SetSlowCmdThreadPoolFlagin the constructor appears to be correct and ensures that the thread pool is initialized based on the configuration provided byg_pika_conf->slow_cmd_pool().No further verification is needed as the function is correctly used to initialize the thread pool based on the configuration.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `SetSlowCmdThreadPoolFlag` correctly initializes the thread pool based on the configuration. # Test: Search for the method usage and configuration handling. rg --type cpp --context 5 'SetSlowCmdThreadPoolFlag'Length of output: 2186
| @@ -707,17 +721,13 @@ void PikaServer::SetFirstMetaSync(bool v) { | |||
| } | |||
|
|
|||
| void PikaServer::ScheduleClientPool(net::TaskFunc func, void* arg, bool is_slow_cmd) { | |||
| if (is_slow_cmd) { | |||
| if (is_slow_cmd && g_pika_conf->slow_cmd_pool()) { | |||
There was a problem hiding this comment.
Add safety checks to ensure pika_slow_cmd_thread_pool_ is initialized before scheduling tasks in ScheduleClientPool.
+ if (!pika_slow_cmd_thread_pool_) {
+ LOG(WARNING) << "Slow command thread pool is not initialized";
+ return;
+ }
if (is_slow_cmd && g_pika_conf->slow_cmd_pool()) {
pika_slow_cmd_thread_pool_->Schedule(func, arg);
return;
}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.
| if (is_slow_cmd && g_pika_conf->slow_cmd_pool()) { | |
| if (!pika_slow_cmd_thread_pool_) { | |
| LOG(WARNING) << "Slow command thread pool is not initialized"; | |
| return; | |
| } | |
| if (is_slow_cmd && g_pika_conf->slow_cmd_pool()) { | |
| pika_slow_cmd_thread_pool_->Schedule(func, arg); | |
| return; | |
| } |
| void PikaServer::SetSlowCmdThreadPoolFlag(bool flag) { | ||
| slow_cmd_thread_pool_flag_ = flag; | ||
| int ret = 0; | ||
| if (flag) { | ||
| ret = pika_slow_cmd_thread_pool_->start_thread_pool(); | ||
| if (ret != net::kSuccess) { | ||
| dbs_.clear(); | ||
| LOG(ERROR) << "Start PikaLowLevelThreadPool Error: " << ret | ||
| << (ret == net::kCreateThreadError ? ": create thread error " : ": other error"); | ||
| } | ||
| } else { | ||
| while (SlowCmdThreadPoolCurQueueSize() != 0) { | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); | ||
| } | ||
| pika_slow_cmd_thread_pool_->stop_thread_pool(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider enhancing error handling in SetSlowCmdThreadPoolFlag to include recovery mechanisms or retries when starting the thread pool fails.
+ try {
ret = pika_slow_cmd_thread_pool_->start_thread_pool();
if (ret != net::kSuccess) {
dbs_.clear();
LOG(ERROR) << "Start PikaLowLevelThreadPool Error: " << ret
<< (ret == net::kCreateThreadError ? ": create thread error " : ": other error");
}
+ } catch (const std::exception& e) {
+ LOG(ERROR) << "Exception caught while starting thread pool: " << e.what();
+ // Implement recovery or retry logic here
+ }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 PikaServer::SetSlowCmdThreadPoolFlag(bool flag) { | |
| slow_cmd_thread_pool_flag_ = flag; | |
| int ret = 0; | |
| if (flag) { | |
| ret = pika_slow_cmd_thread_pool_->start_thread_pool(); | |
| if (ret != net::kSuccess) { | |
| dbs_.clear(); | |
| LOG(ERROR) << "Start PikaLowLevelThreadPool Error: " << ret | |
| << (ret == net::kCreateThreadError ? ": create thread error " : ": other error"); | |
| } | |
| } else { | |
| while (SlowCmdThreadPoolCurQueueSize() != 0) { | |
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); | |
| } | |
| pika_slow_cmd_thread_pool_->stop_thread_pool(); | |
| } | |
| } | |
| void PikaServer::SetSlowCmdThreadPoolFlag(bool flag) { | |
| slow_cmd_thread_pool_flag_ = flag; | |
| int ret = 0; | |
| if (flag) { | |
| try { | |
| ret = pika_slow_cmd_thread_pool_->start_thread_pool(); | |
| if (ret != net::kSuccess) { | |
| dbs_.clear(); | |
| LOG(ERROR) << "Start PikaLowLevelThreadPool Error: " << ret | |
| << (ret == net::kCreateThreadError ? ": create thread error " : ": other error"); | |
| } | |
| } catch (const std::exception& e) { | |
| LOG(ERROR) << "Exception caught while starting thread pool: " << e.what(); | |
| // Implement recovery or retry logic here | |
| } | |
| } else { | |
| while (SlowCmdThreadPoolCurQueueSize() != 0) { | |
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); | |
| } | |
| pika_slow_cmd_thread_pool_->stop_thread_pool(); | |
| } | |
| } |
| @@ -749,11 +759,13 @@ | |||
| } | |||
|
|
|||
| void PikaServer::BGSaveTaskSchedule(net::TaskFunc func, void* arg) { | |||
| bgsave_thread_.set_thread_name("BGSaveTask"); | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Add error handling or logging after starting the thread in BGSaveTaskSchedule to ensure the thread starts successfully.
- File:
src/pika_server.cc - Lines: 762-762
Analysis chain
Verify that the thread successfully starts in BGSaveTaskSchedule before scheduling tasks.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the thread starts successfully in `BGSaveTaskSchedule`.
# Test: Search for error handling after thread start.
rg --type cpp --context 5 'bgsave_thread_.StartThread()'
Length of output: 1153
| @@ -844,6 +853,7 @@ class PikaConf : public pstd::BaseConf { | |||
| std::string bgsave_path_; | |||
| std::string bgsave_prefix_; | |||
| std::string pidfile_; | |||
| std::atomic<bool> slow_cmd_pool_; | |||
There was a problem hiding this comment.
原子变量不用加锁保护，不要使用原子变量
done
src/net/src/holy_thread.cc
Outdated
| @@ -75,6 +75,7 @@ int HolyThread::StartThread() { | |||
| if (ret) { | |||
| return ret; | |||
| } | |||
| set_thread_name("HolyThread"); | |||
There was a problem hiding this comment.
HolyThread是其他类的父类，你在这里设置。会不会其他类设置的名字被这个替代了。
There was a problem hiding this comment.
HolyThread是其他类的父类，你在这里设置。会不会其他类设置的名字被这个替代了。
已修复
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- conf/pika.conf (1 hunks)
- include/pika_conf.h (3 hunks)
- include/pika_server.h (4 hunks)
- src/pika_admin.cc (3 hunks)
- src/pika_conf.cc (2 hunks)
- src/pika_server.cc (13 hunks)
Files skipped from review as they are similar to previous changes (5)
- conf/pika.conf
- include/pika_conf.h
- src/pika_admin.cc
- src/pika_conf.cc
- src/pika_server.cc
Additional comments not posted (1)
include/pika_server.h (1)
651-651: Ensureslow_cmd_thread_pool_flag_is properly initialized and thread-safe.
| @@ -97,6 +97,7 @@ class PikaServer : public pstd::noncopyable { | |||
| bool force_full_sync(); | |||
| void SetForceFullSync(bool v); | |||
| void SetDispatchQueueLimit(int queue_limit); | |||
| void SetSlowCmdThreadPoolFlag(bool flag); | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure thread safety when toggling slow_cmd_thread_pool_flag_. The current implementation lacks mutex locks or atomic operations, which could lead to race conditions.
src/pika_server.cc:PikaServer::SetSlowCmdThreadPoolFlag
Analysis chain
Ensure thread safety when toggling slow_cmd_thread_pool_flag_.
Ensure thread safety around slow_cmd_thread_pool_flag_ usage by checking for mutex locks or atomic operations.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential race conditions related to `slow_cmd_thread_pool_flag_`.
# Test: Search for occurrences of `slow_cmd_thread_pool_flag_` being accessed.
rg --type cpp --context 5 'slow_cmd_thread_pool_flag_' src/
Length of output: 1088
Script:
#!/bin/bash
# Description: Check for thread safety mechanisms around `slow_cmd_thread_pool_flag_` usage.
# Test: Search for mutex locks or atomic operations related to `slow_cmd_thread_pool_flag_`.
rg --type cpp --context 5 'slow_cmd_thread_pool_flag_' src/ | grep -E 'std::mutex|std::lock_guard|std::atomic'
Length of output: 110
chejinge
force-pushed
the
feat_thread_purge
branch
from
9d7d145
to
49b0358
Compare
src/net/src/net_thread.cc
Outdated
| @@ -17,7 +17,7 @@ Thread::~Thread() = default; | |||
| void* Thread::RunThread(void* arg) { | |||
| auto thread = reinterpret_cast<Thread*>(arg); | |||
| if (!(thread->thread_name().empty())) { | |||
| SetThreadName(pthread_self(), thread->thread_name()); | |||
| SetThreadName(pthread_self(), "pika"); | |||
There was a problem hiding this comment.
这里直接改成pika不合适吧？Thread应该是个基类，之前的那种方式看起来更合理点。
There was a problem hiding this comment.
这里直接改成pika不合适吧？Thread应该是个基类，之前的那种方式看起来更合理点。
这个代码 没有吧 应该已经没有改
| res_.AppendStringRaw( "-ERR Invalid argument \'" + value + "\' for CONFIG SET 'slow-cmd-pool'\r\n"); | ||
| return; | ||
| } | ||
| g_pika_conf->SetSlowCmdPool(SlowCmdPool); |
There was a problem hiding this comment.
你schedule判断是否把请求塞给慢队列的时候，会读取g_pika_conf->slow_cmd_pool，setslowcmdthreadpoolflag中会等待慢队列为空然后才会stop线程池，那么这里就应该先改pika conf，保证不再有新的请求塞到队列里，然后你在等待队列都消费完之后返回。
There was a problem hiding this comment.
setslowcmdthreadpoolflag
done
chejinge
force-pushed
the
feat_thread_purge
branch
from
49b0358
to
8bac0c6
Compare
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/pika_admin.cc (3 hunks)
- src/rsync_client.cc (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/pika_admin.cc
- src/rsync_client.cc
* feat:thread purge --------- Co-authored-by: chejinge <chejinge@360.cn>
快慢命令线程池的退出会判断队列的大小是否为0，不为0会循环去等待直到为0，线程池会退出
Summary by CodeRabbit
New Features
slow-cmd-poolto control the separation of fast and slow commands with optionsyesorno.Improvements
Removals
SetLoopDBStateMachinefrom thePikaServerclass.Bug Fixes