reimplement the ‘monitor' command by delete its thread #1551

loveyacper · 2023-05-25T03:57:47Z

No description provided.

cr-gpt · 2023-05-25T03:57:54Z

include/pika_server.h

@@ -411,7 +411,8 @@ class PikaServer {
  /*
   * Monitor used
   */
-  std::unique_ptr<PikaMonitorThread> pika_monitor_thread_;
+  mutable pstd::Mutex monitor_mutex_protector_;
+  std::set<std::weak_ptr<PikaClientConn>, std::owner_less<std::weak_ptr<PikaClientConn>>> pika_monitor_clients_;

  /*
   * Rsync used


The code patch removes the PikaMonitorThread class and adds a std::set data member to keep track of monitor clients. The HasMonitorClients() method is also updated to be marked const. Additionally, a mutex is added to protect access to the pika_monitor_clients_ set.

It's difficult to provide more detailed feedback without seeing the entirety of the codebase and how this change fits into it. However, some suggestions for further improvement could include verifying that the changes have been thoroughly tested, and possibly implementing move semantics for the PikaClientConn type to improve performance when inserting elements into the std::set.

cr-gpt · 2023-05-25T03:57:59Z

src/pika_admin.cc

-  return;  // Monitor thread will return "OK"
+
+  g_pika_server->AddMonitorClient(std::dynamic_pointer_cast<PikaClientConn>(conn_repl));
+  res_.SetRes(CmdRes::kOk);
 }

 void DbsizeCmd::DoInitial() {


It seems that a block of code has been removed in the MonitorCmd::Do function that was responsible for moving the connection from one thread to another thread. It would be better to review why this change was made and if it has any impact on the functionality.

Also, the current implementation assumes that the cast std::dynamic_pointer_cast<PikaClientConn>(conn_repl) always succeeds, so it might be worth adding some additional checks or error handling logic.

Regarding the DbsizeCmd::DoInitial function, nothing stands out as problematic or in need of improvement based on the provided code patch.

cr-gpt · 2023-05-25T03:58:06Z

src/pika_server.cc

 }

 void PikaServer::AddMonitorClient(std::shared_ptr<PikaClientConn> client_ptr) {
-  pika_monitor_thread_->AddMonitorClient(client_ptr);
+  std::unique_lock lock(monitor_mutex_protector_);
+  pika_monitor_clients_.insert(client_ptr);
 }

 void PikaServer::SlowlogTrim() {


The code patch removes the PikaMonitorThread object and its associated functions, replacing it with a new approach to broadcast monitor messages to connected clients.

Improvement suggestions:

Add comments explaining the purpose of each function.

Use std::shared_ptr instead of raw pointers to manage memory in AddMonitorClient.

Consider adding error handle for the std::unique_lock calls in AddMonitorClient, HasMonitorClients, and AddMonitorMessage.

Consider adding a timeout for the monitor message delivery mechanism to avoid blocking in case of client disconnections or network issues.

The code seems to have removed some pika_monitor_thread_ related functionalities but did not update some of the functions called by PikaServer. Ensure any unnecessary elements are removed from the code.

cr-gpt · 2023-05-25T04:03:16Z

include/pika_server.h

@@ -411,7 +411,8 @@ class PikaServer {
  /*
   * Monitor used
   */
-  std::unique_ptr<PikaMonitorThread> pika_monitor_thread_;
+  mutable pstd::Mutex monitor_mutex_protector_;
+  std::set<std::weak_ptr<PikaClientConn>, std::owner_less<std::weak_ptr<PikaClientConn>>> pika_monitor_clients_;

  /*
   * Rsync used


The code patch adds a std::set to keep track of monitor clients and replaces the non-const bool HasMonitorClients() function with a const version. Additionally, a new mutex called monitor_mutex_protector_ is added to protect the monitor thread and its clients.

Overall, the changes seem reasonable and should not introduce any bugs. However, it would be useful to see the PikaMonitorThread class to ensure that the modifications are consistent with the overall design.

A minor improvement suggestion would be to use std::shared_ptr instead of std::weak_ptr in pika_monitor_clients_, which would prevent dangling pointer issues if the client object is destroyed while still being accessed from the set. While the owner_less is used to sort std::weak_ptr, it only guarantees strict weak ordering among expired pointers, but does not check for expiring pointers when inserting/deleting, making "use-after-free" bugs possible.

cr-gpt · 2023-05-25T04:03:21Z

src/pika_admin.cc

-  return;  // Monitor thread will return "OK"
+
+  g_pika_server->AddMonitorClient(std::dynamic_pointer_cast<PikaClientConn>(conn_repl));
+  res_.SetRes(CmdRes::kOk);
 }

 void DbsizeCmd::DoInitial() {


It seems that the patch removed some code related to moving a network connection out of a server thread, replacing it with directly adding a client to the monitor and setting a response to "OK". As long as it is intended to remove that specific functionality, there shouldn't be any bug risks. However, without knowing the context of the application, it's hard to suggest any improvements.

cr-gpt · 2023-05-25T04:03:26Z

src/pika_server.cc

+  if (client_ptr) {
+    std::unique_lock lock(monitor_mutex_protector_);
+    pika_monitor_clients_.insert(client_ptr);
+  }
 }

 void PikaServer::SlowlogTrim() {


The code patch removes the pika_monitor_thread_ member variable and related functions, and instead introduces a monitor_mutex_protector_ and uses a new container pika_monitor_clients_ to store monitor clients.

This change could potentially reduce complexity and improve thread safety. However, it is impossible to assess bug risk without knowledge of the broader system context and testing.

One improvement suggestion would be to use std::shared_ptr instead of raw pointers or weak_ptr when managing client connections, as this could improve memory management and simplify the code.

ghost · 2023-05-25T14:34:46Z

include/pika_server.h

@@ -411,7 +411,8 @@ class PikaServer {
  /*
   * Monitor used
   */
-  std::unique_ptr<PikaMonitorThread> pika_monitor_thread_;
+  mutable pstd::Mutex monitor_mutex_protector_;


is better to use RWLock?

Make no difference.

ghost · 2023-05-25T14:56:12Z

src/pika_server.cc

+    auto cli = (*it).lock();
+    if (cli) {
+      cli->WriteResp(msg);
+      auto status = cli->SendReply();


it's possible SendReply not in lock?

Ok, it's a improvement if there exists too many monitor clients, fix it now.

cr-gpt · 2023-05-26T02:00:53Z

include/pika_server.h

@@ -411,7 +411,8 @@ class PikaServer {
  /*
   * Monitor used
   */
-  std::unique_ptr<PikaMonitorThread> pika_monitor_thread_;
+  mutable pstd::Mutex monitor_mutex_protector_;
+  std::set<std::weak_ptr<PikaClientConn>, std::owner_less<std::weak_ptr<PikaClientConn>>> pika_monitor_clients_;

  /*
   * Rsync used


The code patch includes adding a set of monitor clients to PikaServer class, replacing the old implementation using PikaMonitorThread. The new implementation uses a mutex to protect the access to the set, and the set contains weak pointers to PikaClientConn objects instead of shared pointers. No bug risk is apparent from this patch.

A suggestion for improvement is to use a shared_mutex instead of a Mutex for monitor_mutex_protector_, as multiple read accesses can be made safely and concurrently.

cr-gpt · 2023-05-26T02:01:01Z

src/pika_admin.cc

-  return;  // Monitor thread will return "OK"
+
+  g_pika_server->AddMonitorClient(std::dynamic_pointer_cast<PikaClientConn>(conn_repl));
+  res_.SetRes(CmdRes::kOk);
 }

 void DbsizeCmd::DoInitial() {


The code patch you provided removes some code that moves a connection object from one thread to another and replaces it with a simpler approach of adding the connection directly to the monitor client list. This seems like a reasonable simplification if it meets the requirements of the application.

One potential improvement could be to add error handling for the case where conn_repl is not actually an instance of PikaClientConn. You could add a check before casting it to PikaClientConn to avoid a potential runtime error.

For the DbsizeCmd function, no changes are shown, so it's unclear what the purpose of this function is. It may be worthwhile to review this function as well, especially if it relates to the functionality being changed in the MonitorCmd function.

cr-gpt · 2023-05-26T02:01:16Z

src/pika_server.cc

+  if (client_ptr) {
+    std::unique_lock lock(monitor_mutex_protector_);
+    pika_monitor_clients_.insert(client_ptr);
+  }
 }

 void PikaServer::SlowlogTrim() {


The code review suggests the following:

Removing pika_monitor_thread_ object creation as it is no longer being used.

In ClientKillAll(), remove the call to pika_monitor_thread_->ThreadClientKill() since the object is being removed and this function is no longer needed.

In ClientKill(), remove the call to pika_monitor_thread_->ThreadClientKill(ip_port) since the object is being removed and this function is no longer needed.

In ClientList(), remove the call to pika_monitor_thread_->ThreadClientList(clients) since the object is being removed and this function is no longer needed.

Replace pika_monitor_thread_->HasMonitorClients() with a new implementation HasMonitorClients() that uses a mutex-protected set of pika_monitor_clients_ to check if there are any monitor clients connected. Lock the mutex before accessing the set.

Add a monitor_mutex_protector_ mutex to protect access to the pika_monitor_clients_ set.

In AddMonitorMessage(), send the message to all connected monitor clients by iterating over the pika_monitor_clients_ set, passing each client the message as a string, constructing a +-prefixed, CR LF-terminated Redis protocol message format. Lock the monitor_mutex_protector_ before iterating over the set.

In AddMonitorClient(), add the given client_ptr to the pika_monitor_clients_ set. Lock the monitor_mutex_protector_ before adding the client.

wanghenshui

LGTM

cr-gpt bot reviewed May 25, 2023

View reviewed changes

AlexStocks requested review from wanghenshui, lqxhub, Supowang1989, chejinge and a user and removed request for Supowang1989 May 25, 2023 13:43

lqxhub approved these changes May 25, 2023

View reviewed changes

ghost reviewed May 25, 2023

View reviewed changes

refactor: implement monitor without thread

c750fd3

cr-gpt bot reviewed May 26, 2023

View reviewed changes

ghost approved these changes May 26, 2023

View reviewed changes

wanghenshui approved these changes May 26, 2023

View reviewed changes

AlexStocks changed the title ~~去除线程，实现 monitor 命令~~ reimplement the ‘monitor' command by delete its thread May 26, 2023

AlexStocks merged commit cf0300d into OpenAtomFoundation:unstable May 26, 2023
8 checks passed

loveyacper deleted the refactor-monitor-thread branch July 28, 2023 10:31

bigdaronlee163 pushed a commit to bigdaronlee163/pika that referenced this pull request Jun 8, 2024

refactor: implement monitor without thread (OpenAtomFoundation#1551)

8d3da11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reimplement the ‘monitor' command by delete its thread #1551

reimplement the ‘monitor' command by delete its thread #1551

loveyacper commented May 25, 2023

cr-gpt bot May 25, 2023

cr-gpt bot May 25, 2023

cr-gpt bot May 25, 2023

cr-gpt bot May 25, 2023

cr-gpt bot May 25, 2023

cr-gpt bot May 25, 2023

ghost May 25, 2023

loveyacper May 26, 2023

ghost May 25, 2023

loveyacper May 26, 2023 •

edited

cr-gpt bot May 26, 2023

cr-gpt bot May 26, 2023

cr-gpt bot May 26, 2023

wanghenshui left a comment

reimplement the ‘monitor' command by delete its thread #1551

reimplement the ‘monitor' command by delete its thread #1551

Conversation

loveyacper commented May 25, 2023

cr-gpt bot May 25, 2023

Choose a reason for hiding this comment

cr-gpt bot May 25, 2023

Choose a reason for hiding this comment

cr-gpt bot May 25, 2023

Choose a reason for hiding this comment

cr-gpt bot May 25, 2023

Choose a reason for hiding this comment

cr-gpt bot May 25, 2023

Choose a reason for hiding this comment

cr-gpt bot May 25, 2023

Choose a reason for hiding this comment

ghost May 25, 2023

Choose a reason for hiding this comment

loveyacper May 26, 2023

Choose a reason for hiding this comment

ghost May 25, 2023

Choose a reason for hiding this comment

loveyacper May 26, 2023 • edited

Choose a reason for hiding this comment

cr-gpt bot May 26, 2023

Choose a reason for hiding this comment

cr-gpt bot May 26, 2023

Choose a reason for hiding this comment

cr-gpt bot May 26, 2023

Choose a reason for hiding this comment

wanghenshui left a comment

Choose a reason for hiding this comment

loveyacper May 26, 2023 •

edited