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 3: Code-Review+1 (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 code to have one base class like this? class ImpalaBaseServerSocket { public: 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: virtual void setKeepAlive(bool enabled) = 0; 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 ImpalaTlsServerSocket : public apache::thrift::transport::TSSLServerSocket, public ImpalaBaseServerSocket { public: using TSSLServerSocket::TSSLServerSocket; protected: void setKeepAlive(bool enabled) override { apache::thrift::transport::TSSLServerSocket::setKeepAlive(enabled); } std::shared_ptr<apache::thrift::transport::TSocket> createSocket(THRIFT_SOCKET socket); }; ... Or maybe use template like this? template <typename SocketType> class ImpalaServerSocket : public SocketType { public: using SocketType::SocketType; void setKeepAliveOptions(int32_t probe_period_s, int32_t retry_period_s, int32_t retry_count) { keepalive_enabled_ = probe_period_s > 0; SocketType::setKeepAlive(keepalive_enabled_); keepalive_probe_period_s_ = probe_period_s; keepalive_retry_period_s_ = retry_period_s; keepalive_retry_count_ = retry_count; } protected: 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 ImpalaTlsServerSocket : public ImpalaServerSocket<apache::thrift::transport::TSSLServerSocket> { public: using ImpalaServerSocket::ImpalaServerSocket; protected: std::shared_ptr<apache::thrift::transport::TSocket> createSocket(THRIFT_SOCKET socket); }; ... 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 duplication, depending on whether we refactor in thrift-util.h -- 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 14:31:25 +0000 Gerrit-HasComments: Yes
