Todd Lipcon 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 5: (12 comments) http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.cc File src/kudu/hms/mini_hms.cc: http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.cc@68 PS5, Line 68: CHECK(!krb5_conf.empty()); worth CHECK(!hms_process_) here too to ensure that it's set up before starting http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.cc@240 PS5, Line 240: : <property> : <name>hadoop.rpc.protection</name> : <value>$5</value> : </property> odd this is in hive-site and not core-site... but guess it works 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@53 PS5, Line 53: Width "width" strikes me as odd relative to "Size" http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@157 PS5, Line 157: ResetReadBuf(); are we guaranteed that read_slice_.data() != read_buf_.data()? ie it's always a new internal buffer? can we CHECK it? http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@177 PS5, Line 177: // If we have some data in the read buffer, then return it. : if (!read_slice_.empty()) { : return read_fast(); : } : : // Otherwise, read a new frame from the wire and return it. : ReadFrame(); : return read_fast(); this code is a bit odd to me. why not just do: if (read_slice_.empty()) { ReadFrame(); } // contents of read_fast go here http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@224 PS5, Line 224: ResetWriteBuf(); the shrink_to_fit() in here seems a bit odd -- if you're doing a multi-frame write() call you'll end up flushing and reallocating many times, right? http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.h File src/kudu/rpc/sasl_common.h: http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.h@116 PS5, Line 116: // The ciphertext data must not be greater than the maximum buffer size. not sure how to interpret this comment, since ciphertext is an out-param http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.h@122 PS5, Line 122: Slice* ciphertext) WARN_UNUSED_RESULT; think this param needs a bit more doc - it's an in-out param, right? is it guaranteed that it still point to the same spot on return or could it write to some other buffer? http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.h@126 PS5, Line 126: // The encoded data must not be greater than the maximum buffer size. same http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc File src/kudu/rpc/sasl_common.cc: http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc@48 PS5, Line 48: extern const size_t kSaslMaxOutBufLen = 64 * 1024; if this is our max frame length for thrift connections to the HMS, is this going to blow up when we try to use the "pubsub" thing which apparently often returns huge responses? Or will a single call response be appropriately fragmented into smaller frames? http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc@356 PS5, Line 356: No max output buffer size wrong message here http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc@367 PS5, Line 367: Status SaslEncode(sasl_conn_t* conn, Slice plaintext, Slice* ciphertext) { we seem to have lost the looping behavior in the case that the input is larger than the buffer. Can we at least return a bad Status or CHECK if you don't want to keep that implementation? -- 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: 5 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: Wed, 24 Jan 2018 22:20:38 +0000 Gerrit-HasComments: Yes
