mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Till Toenshoff <toensh...@me.com>
Subject Re: Review Request 51605: Added "mesos-tcp-connect" binary.
Date Wed, 02 Nov 2016 11:05:16 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message