impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-1928: Fix Thrift client transport wrapping order
Date Tue, 17 May 2016 05:02:32 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-1928: Fix Thrift client transport wrapping order
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3093/1/be/src/rpc/thrift-client.h
File be/src/rpc/thrift-client.h:

Line 153: transport_
> why not just pass socket_ here?  do we purposely want transport_ to be sock
I had the same question for Mostafa-- he had said it was to avoid a really ugly cast. He's
right about the ugly cast-- it's especially bad here in the header where we use the full namespaces.

Great point about the error handling. I guess in the past if this failed then transport_ would
still have had something non-NULL and this client may have limped along, though probably not
really as a functional client if the server-side expected a Sasl connection.

We absolutely should handle this error, though now I'm worried about making changes we don't
have the capability to extensively test. I think that unfortunately we may want to try to
maintain the old behavior (basically what is here) for this release since this is as close
as possible to the code that has already shipped in a few releases (this has been here since
C5.5), and then file a critical JIRA to address this immediately, and with test coverage.
Of course this is purely in the interest of making fewer changes that we can't test well so
close to the release...

(In fact, the follow-up work should actually move the initialization logic that can fail into
an Init() method.)

What do you think?


-- 
To view, visit http://gerrit.cloudera.org:8080/3093
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I81d30b3d8d10fe6dcd8eb88cca49734af09f9d91
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message