Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/22254 )
Change subject: IMPALA-13253: Add option to enable keepalive for client connections ...................................................................... Patch Set 7: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/22254/3/be/src/rpc/thrift-util.h File be/src/rpc/thrift-util.h: http://gerrit.cloudera.org:8080/#/c/22254/3/be/src/rpc/thrift-util.h@189 PS3, Line 189: // These options are only effective if keepalive is enabled separately (by Thrift). : Status SetKeepAliveOptionsForSocket(THRIFT_SOCKET, int32_t probe_period_s, : int32_t retry_period_s, int32_t retry_count); : : template <typename ThriftServerSocketType> : class ImpalaKeepAliveServerSocket : public ThriftServerSocketType { : public: : using ThriftServerSocketType::ThriftServerSocketType; : : void setKeepAliveOptions(int32_t probe_period_s, int32_t retry_period_s, : int32_t retry_count) { : keepalive_enabled_ = probe_period_s > 0; : ThriftServerSocketType::setKeepAlive(keepalive_enabled_); : keepalive_probe_period_s_ = probe_period_s; : keepalive_retry_period_s_ = retry_period_s; : keepalive_retry_count_ = retry_count; : } : : protected: : std::shared_ptr<apache::thrift::transport::TSocket> createSocket(THRIFT_SOCKET socket) { : std::shared_ptr<apache::thrift::transport::TSocket> tsocket = : ThriftServerSocketType::createSocket(socket); : if (keepalive_enabled_) { : Status status = SetKeepAliveOptionsForSocket(socket, keepalive_probe_period_s_, : keepalive_retry_period_s_, keepalive_retry_count_); : if (!status.ok()) { : throw apache::thrift::transport::TTransportException( : apache::thrift::transport::TTransportException::INTERNAL_ERROR, : status.msg().msg()); : } : } : return tsocket; : } : : private: : bool keepalive_enabled_ = false; : int32_t keepalive_probe_period_s_ = 0; : int32_t keepalive_retry_period_s_ = 0; : int32_t keepalive_retry_count_ = 0; : }; : : /// Redirects all Thrift logging to VLOG(1) : vo > Switched to a templated implementation. Let me know what you think. Thanks, Joe. It looks good. Maybe we could also add some simple comments to describe the class ImpalaKeepAliveServerSocket and the methods, and then it will look perfect to me -- To view, visit http://gerrit.cloudera.org:8080/22254 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e50f263006c456bc0797b8306aa4065e9713450 Gerrit-Change-Number: 22254 Gerrit-PatchSet: 7 Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Thu, 16 Jan 2025 01:41:31 +0000 Gerrit-HasComments: Yes
