Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/24786 )
Change subject: IMPALA-15257: Add option to use keepalive for internal cluster Thrift connections ...................................................................... Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/24786/6/be/src/rpc/thrift-util.h File be/src/rpc/thrift-util.h: http://gerrit.cloudera.org:8080/#/c/24786/6/be/src/rpc/thrift-util.h@235 PS6, Line 235: SetKeepAliveOptionsForSocket(socket, keepalive_probe_period_s_, : keepalive_retry_period_s_, keepalive_retry_count_) Should the client and server use the same keepalive values? http://gerrit.cloudera.org:8080/#/c/24786/6/be/src/rpc/thrift-util.h@238 PS6, Line 238: throw apache::thrift::transport::TTransportException( : apache::thrift::transport::TTransportException::INTERNAL_ERROR, : status.msg().msg()); As the commit message mentions, "Failures are logged as warnings and do not break connection establishment". Even though this is existing code in the server side, should we also change to log the failure instead of breaking the connection only for the SetKeepAliveOptionsForSocket failure? So it aligns the behavior of server with client http://gerrit.cloudera.org:8080/#/c/24786/6/be/src/rpc/thrift-util.cc File be/src/rpc/thrift-util.cc: http://gerrit.cloudera.org:8080/#/c/24786/6/be/src/rpc/thrift-util.cc@264 PS6, Line 264: LOG(WARNING) : << "ApplyInternalClientKeepAlive called " : "before the socket was opened. TCP_KEEP* tuning would be silently dropped"; Should we return an error status here? If the file descriptor is invalid, calling SetKeepAliveOptionsForSocket() will probably trigger another unnecessary failed SetSockOpt syscall -- To view, visit http://gerrit.cloudera.org:8080/24786 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If9fec7a02b2c5ef92e8b68de9e3993bcee7bc898 Gerrit-Change-Number: 24786 Gerrit-PatchSet: 6 Gerrit-Owner: Jiyoung Yoo <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jiyoung Yoo <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Thu, 10 Sep 2026 03:02:17 +0000 Gerrit-HasComments: Yes
