[2/4] incubator-impala git commit: IMPALA-1928: Fix Thrift client transport wrapping order

[ta...@apache.org]

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 <dh...@cloudera.com>
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 <mj...@cloudera.com>
Authored: Mon May 16 17:22:42 2016 -0700
Committer: Tim Armstrong <ta...@cloudera.com>
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_));