Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/9818 )
Change subject: KUDU-2351 Add IP/port for Recv() failure ...................................................................... Patch Set 7: (10 comments) http://gerrit.cloudera.org:8080/#/c/9818/6/src/kudu/util/net/socket-test.cc File src/kudu/util/net/socket-test.cc: http://gerrit.cloudera.org:8080/#/c/9818/6/src/kudu/util/net/socket-test.cc@48 PS6, Line 48: virtual void SetUp() override { > Can't you bind to port 0 so it picks an empty one, and then report back the Done http://gerrit.cloudera.org:8080/#/c/9818/6/src/kudu/util/net/socket-test.cc@53 PS6, Line 53: for (auto& thread : threads) { > nit: too much indentation Done http://gerrit.cloudera.org:8080/#/c/9818/6/src/kudu/util/net/socket-test.cc@59 PS6, Line 59: > nit: indent by 1 Done http://gerrit.cloudera.org:8080/#/c/9818/6/src/kudu/util/net/socket-test.cc@61 PS6, Line 61: Socket listener_; > style: this member should end in _ Done http://gerrit.cloudera.org:8080/#/c/9818/6/src/kudu/util/net/socket-test.cc@64 PS6, Line 64: d DoTes > style: DoTest since this is a non-inline method Done http://gerrit.cloudera.org:8080/#/c/9818/6/src/kudu/util/net/socket-test.cc@66 PS6, Line 66: td::future<Sockaddr> sockaddr_future = sockaddr_promise.get_future(); : threads.emplace_back(std::thread([&]{ : Sockaddr address; : address.ParseString("0.0.0.0", 0); : > nit: messy inentation. Please add CHECK_OK around all the call sites that r Done http://gerrit.cloudera.org:8080/#/c/9818/6/src/kudu/util/net/socket-test.cc@75 PS6, Line 75: listener_.GetSocketAddress(&sockaddr); > we use SleepFor from monotime.h instead Done http://gerrit.cloudera.org:8080/#/c/9818/6/src/kudu/util/net/socket-test.cc@82 PS6, Line 82: sock.Close(); > isn't this racey? how do we know that the other thread is listening yet? I Done http://gerrit.cloudera.org:8080/#/c/9818/6/src/kudu/util/net/socket-test.cc@91 PS6, Line 91: > in order to deal with the ephemeral port you can use ASSERT_STR_MATCHES ins Done http://gerrit.cloudera.org:8080/#/c/9818/6/src/kudu/util/net/socket-test.cc@95 PS6, Line 95: client.SetRecvTimeout(MonoDelta::FromMilliseconds(100)); > You can have it parameterized on std::pair<bool, std::string>, but I'm fine let me know if you think it would be better with a std::pair<bool, std::string>, I can change it, but for me it didn't make sense contextually (on the other hand the more I think about it, the more sense it makes as it's still the same method just different error) -- 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: 7 Gerrit-Owner: Attila Bukor <abu...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 28 Mar 2018 10:49:36 +0000 Gerrit-HasComments: Yes
