Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9818 )
Change subject: KUDU-2351 Add IP/port for Recv() failure ...................................................................... Patch Set 14: (3 comments) http://gerrit.cloudera.org:8080/#/c/9818/14/src/kudu/security/tls_socket-test.cc File src/kudu/security/tls_socket-test.cc: http://gerrit.cloudera.org:8080/#/c/9818/14/src/kudu/security/tls_socket-test.cc@224 PS14, Line 224: server.Stop(); I'm surprised this works, how does the server receive and respond to the write if it's already stopped? I expected the Stop() to have to occur in between the Recv calls. The test isn't failing, though, so I must be misunderstanding something. http://gerrit.cloudera.org:8080/#/c/9818/11/src/kudu/security/tls_socket.cc File src/kudu/security/tls_socket.cc: http://gerrit.cloudera.org:8080/#/c/9818/11/src/kudu/security/tls_socket.cc@111 PS11, Line 111: CHECK(ssl_); > thanks, moved it down under L118, so the save_errno != 0 case in L125 could nice, that's even simpler. http://gerrit.cloudera.org:8080/#/c/9818/14/src/kudu/util/net/socket-test.cc File src/kudu/util/net/socket-test.cc: http://gerrit.cloudera.org:8080/#/c/9818/14/src/kudu/util/net/socket-test.cc@65 PS14, Line 65: threads.emplace_back(std::thread([&]{ Could this thread be a local variable, and join at the end of the test? I think that's easier to follow than having cleanup happen in TearDown(). -- To view, visit http://gerrit.cloudera.org:8080/9818 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I22436b13bb351b132e1c0b7159294dd0c980c2b3 Gerrit-Change-Number: 9818 Gerrit-PatchSet: 14 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 19 Apr 2018 17:05:30 +0000 Gerrit-HasComments: Yes
