Dan Burkert has posted comments on this change. Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation ......................................................................
Patch Set 10: (13 comments) http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/connection.h File src/kudu/rpc/connection.h: PS10, Line 159: take_socket > nit: release_socket ? Done PS10, Line 163: set_socket > nit: adopt_socket ? Done http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/negotiation.h File src/kudu/rpc/negotiation.h: PS10, Line 25: class Socket; > Why is it necessary in here? Done http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/reactor.cc File src/kudu/rpc/reactor.cc: PS10, Line 345: std::unique_ptr<Socket> new_socket; : if (reactor()->messenger()->ssl_enabled()) { : new_socket = reactor()->messenger()->ssl_factory()->CreateSocket(sock.Release(), false); : } else { : new_socket.reset(new Socket(sock.Release())); : } : : // Register the new connection in our map. : *conn = new Connection(this, conn_id.remote(), std::move(new_socket), Connection::CLIENT); : > nit: consider adding an embedded scope for this to signify that new_socket This snippet is being significantly rewritten in another patch in the series, so I'm going to punt on this for now. http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/sasl_helper.h File src/kudu/rpc/sasl_helper.h: PS10, Line 30: namespace google { : namespace protobuf { : class MessageLite; : } // namespace protobuf : } // namespace google > nit: is this needed? Done PS10, Line 38: class MonoTime; > nit: is this needed? Done http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: PS10, Line 20: #include <sasl/sasl.h> > nit: could you add a comment about the reason of putting this before the bl woops, that was a mistake. PS10, Line 56: * > nit: since the asterisk disposition for other pointer-like parameters has b Done PS10, Line 61: * > ditto Done PS10, Line 453: E > nit: I'm not sure whether we have already adopted the idea of starting thos Changed to lowercase. http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/server_negotiation.h File src/kudu/rpc/server_negotiation.h: PS10, Line 101: take_ > nit: consider renaming into 'release_socket' Done http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/security/ssl_socket.cc File src/kudu/security/ssl_socket.cc: PS10, Line 126: (*nwritten < frame_size) > Is it possible for in-the-middle Write() to return an error even if *nwritt I don't think so, and in case it does, I would expect the next loop through to break out with an error status. PS10, Line 167: SSL_free(ssl_); > Is it possible that an object of this type is destructed without prior call I've recently updated it to call Close from the dtor. -- To view, visit http://gerrit.cloudera.org:8080/5760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
