Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9818 )
Change subject: KUDU-2351 Add IP/port for Recv() failure ...................................................................... Patch Set 9: (9 comments) http://gerrit.cloudera.org:8080/#/c/9818/9/src/kudu/util/net/socket-test.cc File src/kudu/util/net/socket-test.cc: http://gerrit.cloudera.org:8080/#/c/9818/9/src/kudu/util/net/socket-test.cc@1 PS9, Line 1: nit: blank line at top of file http://gerrit.cloudera.org:8080/#/c/9818/9/src/kudu/util/net/socket-test.cc@45 PS9, Line 45: public: style: indentation seems off in this file. we only indent private/public by 1 space, and then the stuff underneath them an additional 1 space, so that the total indentation is 2 within the class http://gerrit.cloudera.org:8080/#/c/9818/9/src/kudu/util/net/socket-test.cc@46 PS9, Line 46: virtual void SetUp() override { : KuduTest::SetUp(); : } is this necessary? http://gerrit.cloudera.org:8080/#/c/9818/9/src/kudu/util/net/socket-test.cc@56 PS9, Line 56: } again strange indent http://gerrit.cloudera.org:8080/#/c/9818/9/src/kudu/util/net/socket-test.cc@63 PS9, Line 63: std::promise hrm, we don't really use std::promise and std::future elsewhere in the codebase. Can you switch this over to Promise from util/promise.h for now? Maybe we should switch at some point but for now let's prefer consistency http://gerrit.cloudera.org:8080/#/c/9818/9/src/kudu/util/net/socket-test.cc@73 PS9, Line 73: GetSocketAddress should CHECK_OK this http://gerrit.cloudera.org:8080/#/c/9818/9/src/kudu/util/net/socket-test.cc@76 PS9, Line 76: Sockaddr new_addr; : Socket sock; these aren't needed in this scope - they can be inside 'if (accept)' http://gerrit.cloudera.org:8080/#/c/9818/9/src/kudu/util/net/socket-test.cc@80 PS9, Line 80: sock.Close(); also CHECK_OK http://gerrit.cloudera.org:8080/#/c/9818/9/src/kudu/util/net/socket-test.cc@93 PS9, Line 93: SetRecvTimeout here too -- 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: 9 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: Wed, 04 Apr 2018 00:40:21 +0000 Gerrit-HasComments: Yes
