Joe McDonnell 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 3: (2 comments) 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: class ImpalaTlsServerSocket : public apache::thrift::transport::TSSLServerSocket { : public: : using TSSLServerSocket::TSSLServerSocket; : void setKeepAliveOptions(int32_t probe_period_s, int32_t retry_period_s, : int32_t retry_count) { : keepalive_enabled_ = probe_period_s > 0; : 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); : : 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; : }; : : class ImpalaNonTlsServerSocket : public apache::thrift::transport::TServerSocket { : public: : using TServerSocket::TServerSocket; : void setKeepAliveOptions(int32_t probe_period_s, int32_t retry_period_s, : int32_t retry_count) { : keepalive_enabled_ = probe_period_s > 0; : 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); : : 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; : }; > Seems both classes share a lot of similar logic. How about we refactor the Switched to a templated implementation. Let me know what you think. http://gerrit.cloudera.org:8080/#/c/22254/3/be/src/rpc/thrift-util.cc File be/src/rpc/thrift-util.cc: http://gerrit.cloudera.org:8080/#/c/22254/3/be/src/rpc/thrift-util.cc@237 PS3, Line 237: std::shared_ptr<TSocket> ImpalaTlsServerSocket::createSocket(THRIFT_SOCKET socket) { : std::shared_ptr<TSocket> tsocket = TSSLServerSocket::createSocket(socket); : if (keepalive_enabled_) { : Status status = SetKeepAliveOptions(socket, keepalive_probe_period_s_, : keepalive_retry_period_s_, keepalive_retry_count_); : if (!status.ok()) { : throw TTransportException(TTransportException::INTERNAL_ERROR, status.msg().msg()); : } : } : return tsocket; : } : : std::shared_ptr<TSocket> ImpalaNonTlsServerSocket::createSocket(THRIFT_SOCKET socket) { : std::shared_ptr<TSocket> tsocket = TServerSocket::createSocket(socket); : if (keepalive_enabled_) { : Status status = SetKeepAliveOptions(socket, keepalive_probe_period_s_, : keepalive_retry_period_s_, keepalive_retry_count_); : if (!status.ok()) { : throw TTransportException(TTransportException::INTERNAL_ERROR, status.msg().msg()); : } : } : return tsocket; : } > It seems possible to combine into a single function to reduce the code dupl This is now shared via the templated implementation. -- 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: 3 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: Wed, 15 Jan 2025 18:51:49 +0000 Gerrit-HasComments: Yes
