----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51605/#review154533 -----------------------------------------------------------
src/health-check/tcp_connect.cpp (lines 17 - 18) <https://reviews.apache.org/r/51605/#comment224139> Move below that windows block please - the windows includes are pure C stuff in this case and those should go to the top. See our styleguide on "Order of includes". src/health-check/tcp_connect.cpp (line 21) <https://reviews.apache.org/r/51605/#comment224138> alphabetize please src/health-check/tcp_connect.cpp (line 28) <https://reviews.apache.org/r/51605/#comment224137> alphabetize please src/health-check/tcp_connect.cpp (line 75) <https://reviews.apache.org/r/51605/#comment224141> s/targetIP/ip/ s/targetPort/port/ src/health-check/tcp_connect.cpp (line 78) <https://reviews.apache.org/r/51605/#comment224135> I find it very unfortunate that we are using raw `socket` and `sockeraddr_in` here. If only `Address` was not part of libprocess but of stout - VERY unfortunate. Once `Address` was part of stout, I feel there is no reason not to use `Address`, `net::IP`, `net::socket` and `net::connect` anymore. This would buy us implicit Windows compatibility and implicit IPv6 compatibility. Actually, maybe we can directly link against libprocess here -- even though `Address` currently is header only, that may certainly change. So maybe we can link `libprocess.a` and hence wont have a dependency against `libmesos.so/dylib`. Would you consider adding a TODO that says so? src/health-check/tcp_connect.cpp (line 83) <https://reviews.apache.org/r/51605/#comment224143> Missing `endl` src/health-check/tcp_connect.cpp (line 88) <https://reviews.apache.org/r/51605/#comment224140> s/socketFd/socket/ - Till Toenshoff On Oct. 31, 2016, 6:55 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51605/ > ----------------------------------------------------------- > > (Updated Oct. 31, 2016, 6:55 p.m.) > > > Review request for mesos, Avinash sridharan, Benjamin Mahler, Gastón Kleiman, > and haosdent huang. > > > Bugs: MESOS-6119 > https://issues.apache.org/jira/browse/MESOS-6119 > > > Repository: mesos > > > Description > ------- > > To remove dependency on `bash` for TCP health checks, introduce > a separate light-weight binary (without libmesos dependency) for > probing TCP connections. > > > Diffs > ----- > > src/CMakeLists.txt 639f8678ba23c4d9a2ea0bf84fbc3d6fc9286ef3 > src/Makefile.am c2f9e442182110d0b450d4824600a4a791f8cf27 > src/health-check/CMakeLists.txt PRE-CREATION > src/health-check/tcp_connect.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/51605/diff/ > > > Testing > ------- > > See https://reviews.apache.org/r/51607/ > > > Thanks, > > Alexander Rukletsov > >
