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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
