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 6: (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: address.ParseString("127.0.0.1", 11010); > address is used to check the error message, + I need to bind to something. Can't you bind to port 0 so it picks an empty one, and then report back the bound address of the socket from the listener thread back to the main thread? http://gerrit.cloudera.org:8080/#/c/9818/6/src/kudu/util/net/socket-test.cc@53 PS6, Line 53: thread.join(); nit: too much indentation http://gerrit.cloudera.org:8080/#/c/9818/6/src/kudu/util/net/socket-test.cc@59 PS6, Line 59: protected: nit: indent by 1 http://gerrit.cloudera.org:8080/#/c/9818/6/src/kudu/util/net/socket-test.cc@61 PS6, Line 61: Sockaddr address; style: this member should end in _ http://gerrit.cloudera.org:8080/#/c/9818/6/src/kudu/util/net/socket-test.cc@64 PS6, Line 64: do_test style: DoTest since this is a non-inline method http://gerrit.cloudera.org:8080/#/c/9818/6/src/kudu/util/net/socket-test.cc@66 PS6, Line 66: listener_.Init(0); : listener_.BindAndListen(address, 0); : : Sockaddr new_addr; : Socket sock; nit: messy inentation. Please add CHECK_OK around all the call sites that return a Status in case they fail (also below in a few places) http://gerrit.cloudera.org:8080/#/c/9818/6/src/kudu/util/net/socket-test.cc@75 PS6, Line 75: std::this_thread::sleep_for(std::chrono::milliseconds(200)); we use SleepFor from monotime.h instead http://gerrit.cloudera.org:8080/#/c/9818/6/src/kudu/util/net/socket-test.cc@82 PS6, Line 82: ASSERT_OK(client.Connect(address)); isn't this racey? how do we know that the other thread is listening yet? I think the other thread should set a Promise<Sockaddr> with its listening address after binding to port 0, and then you can wait on that promise here to find out where to connect http://gerrit.cloudera.org:8080/#/c/9818/6/src/kudu/util/net/socket-test.cc@91 PS6, Line 91: ASSERT_STR_CONTAINS(s.message().ToString(), message); in order to deal with the ephemeral port you can use ASSERT_STR_MATCHES instead http://gerrit.cloudera.org:8080/#/c/9818/6/src/kudu/util/net/socket-test.cc@95 PS6, Line 95: TEST_F(SocketTest, TestRecvReset) { > I looked into it but I'm not sure if this is what I should use here as I ne yea I think for this case it's easier to just do what he's done here -- 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: 6 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: Tue, 27 Mar 2018 22:07:41 +0000 Gerrit-HasComments: Yes
