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 <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: Tue, 27 Mar 2018 22:07:41 +0000
Gerrit-HasComments: Yes

Reply via email to