Matthew Jacobs has posted comments on this change. Change subject: IMPALA-1928 : Impala ODBC bad performance with Kerberos mechanism ......................................................................
Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/2769/2/be/src/rpc/thrift-client.h File be/src/rpc/thrift-client.h: Line 139 > how could socket_ be not NULL here, given this is the constructor? what cas It looks like socket_ can be NULL if an exception was thrown in CreateSocket() (see the .cc file, that fn is called by ThriftClientImpl's constructor). It looks like some issue with ssl may cause that to happen. It's confusing how roundabout that is. However, if the socket_ is NULL, I don't think we want to attempt to wrap it. It might not cause real issues bc Open() returns with a failure if creating the socket failed (it knows because it checks another hidden variable socket_create_status_ ... blegh :/ ). Line 135: // Below is one line of code in ThriftClientImpl::Close(), : // if (transport_.get != NULL && transport_->isOpen()) transport_->close(); : // Here transport_->isOpen() will call socket_->isOpen(), when socket_ is NULL, : // it will crash This comment no longer applies here (and it was always confusing) Line 144: transport_ = socket_; > is it okay that we now unconditionally set transport_ whereas the old code I think we shouldn't call WrapClientTransport if socket_ is null. Can you just wrap 144 through 151 in the if (socket_ != NULL) block? Line 145: transport_ Easier to read if this is socket_ since that's the underlying transport. Line 146: &transport_ > Can this just be &socket_, and get rid of L144 and make L150 as factory.get I think we want socket_ to still point to the TSocket as I think we refer to the TSocket specifically in some cases after the constructor (e.g. see the socket timeout mutators). Also, the types are different -- the output isn't a TSocket (though a TSocket is a TTransport). -- To view, visit http://gerrit.cloudera.org:8080/2769 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iebcf6457091aef1fc0e5bd1549b3fcbafc5560d9 Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
