IMPALA-1928: Fix Thrift client transport wrapping order The thrift client incorrectly wraps the TSaslTransport around the TBufferedTransport which leads to significant performance issues. (Note that the server-side wraps the transports in the correct order already.)
Currently: TSaslTransport(TBufferedTransport(socket)) Should be: TBufferedTransport(TSaslTransport(socket)) As a result, when we write a structure, we end up doing lots of write calls which hit the TSaslTransport which does no buffering. So it ends up producing output that looks like: [0, 0, 0, 1], <one char>, [0, 0, 0, 1], <one char>, etc. for each individual write call. These end up buffered so we don't get lots of tiny packets on the send side. However, on the receiver side we are doing one recv call per Sasl frame. This patch reorders the wrapping of transports in the thrift client, so that it matches the order on the thrift server which improves exhange performance making it within 10% of non-kerberos. Change-Id: I81d30b3d8d10fe6dcd8eb88cca49734af09f9d91 Reviewed-on: http://gerrit.cloudera.org:8080/3093 Reviewed-by: Dan Hecht <[email protected]> Tested-by: Internal Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/9172f4b8 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/9172f4b8 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/9172f4b8 Branch: refs/heads/master Commit: 9172f4b82444596da182955cda65951b60460501 Parents: f067929 Author: Matthew Jacobs <[email protected]> Authored: Mon May 16 17:22:42 2016 -0700 Committer: Tim Armstrong <[email protected]> Committed: Tue May 17 10:09:06 2016 -0700 ---------------------------------------------------------------------- be/src/rpc/thrift-client.h | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/9172f4b8/be/src/rpc/thrift-client.h ---------------------------------------------------------------------- diff --git a/be/src/rpc/thrift-client.h b/be/src/rpc/thrift-client.h index 347e673..d80caf3 100644 --- a/be/src/rpc/thrift-client.h +++ b/be/src/rpc/thrift-client.h @@ -127,26 +127,33 @@ class ThriftClient : public ThriftClientImpl { template <class InterfaceType> ThriftClient<InterfaceType>::ThriftClient(const std::string& ipaddress, int port, - const std::string& service_name, - AuthProvider* auth_provider, bool ssl) + const std::string& service_name, AuthProvider* auth_provider, bool ssl) : ThriftClientImpl(ipaddress, port, ssl), iface_(new InterfaceType(protocol_)), auth_provider_(auth_provider) { - // Below is one line of code in ThriftClientImpl::Close(), - // if (transport_.get != NULL && transport_->isOpen()) transport_->close(); - // Here transport_->isOpen() will call socker_->isOpen(), when socket_ is NULL, - // it will crash - if (socket_ != NULL) { - ThriftServer::BufferedTransportFactory factory; - transport_ = factory.getTransport(socket_); - } if (auth_provider_ == NULL) { auth_provider_ = AuthManager::GetInstance()->GetInternalAuthProvider(); + DCHECK(auth_provider_ != NULL); + } + + // If socket_ is NULL (because ThriftClientImpl::CreateSocket() failed in the base + // class constructor, nothing else should be constructed. Open()/Reopen() will return + // the error that the socket couldn't be created and the caller should be careful to + // not use the client after that. + // TODO: Move initialization code that can fail into a separate Init() method. + if (socket_ == NULL) { + DCHECK(!socket_create_status_.ok()); + return; } + // transport_ is created by wrapping the socket_ in the TTransport provided by the + // auth_provider_ and then a TBufferedTransport (IMPALA-1928). + transport_ = socket_; auth_provider_->WrapClientTransport(address_.hostname, transport_, service_name, &transport_); + ThriftServer::BufferedTransportFactory factory; + transport_ = factory.getTransport(transport_); protocol_.reset(new apache::thrift::protocol::TBinaryProtocol(transport_)); iface_.reset(new InterfaceType(protocol_));
