[GitHub] [brpc] wwbmmm commented on a diff in pull request #2098: support TCP heartbeat of client

["wwbmmm (via GitHub)" <gi...@apache.org>]

wwbmmm commented on code in PR #2098:
URL: https://github.com/apache/brpc/pull/2098#discussion_r1089663536


##########
src/brpc/socket.h:
##########
@@ -198,6 +198,15 @@ struct SocketOptions {
     std::shared_ptr<AppConnect> app_connect;
     // The created socket will set parsing_context with this value.
     Destroyable* initial_parsing_context;
+
+    // Enable TCP keepalive or not.
+    bool keepalive;

Review Comment:
   这4个参数多次一起出现，是不是可以定义一个结构体（比如SocketKeepaliveOption），简化一下代码
   这里可以放结构体的shared_ptr，指针非空表示启用，减少常态下Socket对象占用的内存。



##########
src/brpc/socket.cpp:
##########
@@ -579,6 +583,71 @@ int Socket::ResetFileDescriptor(int fd) {
         }
     }
 
+    do {

Review Comment:
   这一大段建议单独抽出一个函数



##########
src/brpc/input_messenger.cpp:
##########
@@ -53,6 +53,18 @@ DEFINE_bool(log_connection_close, false,
             "Print log when remote side closes the connection");
 BRPC_VALIDATE_GFLAG(log_connection_close, PassValidate);
 
+DEFINE_bool(socket_keepalive, false,
+            "Enable keepalive of sockets if this value is true");
+
+DEFINE_int32(socket_keepidle_s, -1,

Review Comment:
   这几个缩写有点不好理解，可否改成
   socket_keepalive_idle_s
   socket_keepalive_interval_s
   socket_keepalive_count



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: devnull-unsubscribe@infra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org