Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client ...................................................................... Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc File src/kudu/hms/sasl_client_transport.cc: http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@123 PS5, Line 123: ans > That doesn't catch all exceptions. I didn't realize we could hit non Thrift exceptions in Negotiate(), since SaslException is also a type of TException. http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@274 PS5, Line 274: > I'm not following, in this line needs_wrap_ is being retrieved from the SAS nvm, I misunderstood the fact that quth-int and auth-conf need to be negotiated. I thought we were forcing them on. http://gerrit.cloudera.org:8080/#/c/8692/7/src/kudu/hms/sasl_client_transport.cc File src/kudu/hms/sasl_client_transport.cc: http://gerrit.cloudera.org:8080/#/c/8692/7/src/kudu/hms/sasl_client_transport.cc@141 PS7, Line 141: read_buf_.resize(kFrameHeaderSize); : transport_->readAll(read_buf_.data(), kFrameHeaderSize); Why not just read into a uint32_t here and avoid the resize(kFrameHeaderSize) ? Then in L154, you can do just the single resize and just put both the header and the payload at once into read_buf_. http://gerrit.cloudera.org:8080/#/c/8692/7/src/kudu/hms/sasl_client_transport.cc@205 PS7, Line 205: plaintext.remove_prefix(kFrameHeaderSize); Is this correct? If in L191, we figure that the write_buf_.size() > kSaslMaxBufLen, we first flush() a buffer of kSaslMaxBufLen, and we remove the prefix 'kFrameHeaderSize'. When the loop runs the second time, we're still removing the header prefix again. But the header will not be present in the buffer in second iteration of the loop. Unless I'm missing something. http://gerrit.cloudera.org:8080/#/c/8692/7/src/kudu/hms/sasl_client_transport.cc@217 PS7, Line 217: size_t payload_len = write_buf_.size() - kFrameHeaderSize; Same here, in the second iteration of the loop, we'll be losing 'kFrameHeaderSize' bytes of information. -- To view, visit http://gerrit.cloudera.org:8080/8692 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345 Gerrit-Change-Number: 8692 Gerrit-PatchSet: 7 Gerrit-Owner: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Thu, 01 Feb 2018 22:08:25 +0000 Gerrit-HasComments: Yes
