mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 53460: Refactored network::Address into inet::Address.
Date Mon, 28 Nov 2016 22:08:14 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53460/#review157097
-----------------------------------------------------------


Fix it, then Ship it!





3rdparty/libprocess/include/process/address.hpp (line 69)
<https://reviews.apache.org/r/53460/#comment227520>

    Ditto.



3rdparty/libprocess/include/process/address.hpp (line 80)
<https://reviews.apache.org/r/53460/#comment227516>

    Use switch consistently as other functions?



3rdparty/libprocess/include/process/address.hpp (lines 80 - 81)
<https://reviews.apache.org/r/53460/#comment227519>

    This won't compile on windows. Looks like AF_UNIX is not even defined on windows:
    https://msdn.microsoft.com/en-us/library/windows/desktop/ms740506(v=vs.85).aspx
    
    We should properly wrap UNIX with `#ifndef __WINDOWS__`



3rdparty/libprocess/include/process/address.hpp (line 108)
<https://reviews.apache.org/r/53460/#comment227521>

    Wrap with `#ifndef __WINDOWS__`



3rdparty/libprocess/include/process/address.hpp (line 129)
<https://reviews.apache.org/r/53460/#comment227531>

    +1. We should check on path length here.
    
    Probably introduce a 'create' function?



3rdparty/libprocess/include/process/address.hpp (lines 133 - 140)
<https://reviews.apache.org/r/53460/#comment227530>

    +1 on using union as suggested by James.



3rdparty/libprocess/include/process/address.hpp (line 143)
<https://reviews.apache.org/r/53460/#comment227551>

    Maybe you can store a `network::Address` here?



3rdparty/libprocess/include/process/address.hpp (line 167)
<https://reviews.apache.org/r/53460/#comment227535>

    This is non-POD global constant which might cause trouble during global teardown.
    
    We should either use constexpr here (making sure Address's constructor is constexpr constructor),
or use static method as we did before.



3rdparty/libprocess/include/process/address.hpp (line 169)
<https://reviews.apache.org/r/53460/#comment227536>

    Ditto.



3rdparty/libprocess/include/process/address.hpp (lines 223 - 231)
<https://reviews.apache.org/r/53460/#comment227528>

    +1 on using a union as suggested by James.



3rdparty/libprocess/include/process/socket.hpp (line 432)
<https://reviews.apache.org/r/53460/#comment227553>

    inline is not necessary for template specification? Let's be consistent here (either use
inline for all of them, or none of them).



3rdparty/libprocess/src/socket.cpp (lines 56 - 59)
<https://reviews.apache.org/r/53460/#comment227568>

    `ifndef WINDOWS`



3rdparty/libprocess/src/tests/socket_tests.cpp (line 39)
<https://reviews.apache.org/r/53460/#comment227569>

    Looks like this only works on Linux? should we use a socket path in temp dir. Maybe use
TemporaryDirectoryTest fixture and use `path::join(sandbox.get(), "socket")` as the path?



3rdparty/libprocess/src/tests/ssl_tests.cpp (line 51)
<https://reviews.apache.org/r/53460/#comment227570>

    wrap with ifdef WINDOWS


- Jie Yu


On Nov. 28, 2016, 6:15 a.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53460/
> -----------------------------------------------------------
> 
> (Updated Nov. 28, 2016, 6:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Also added unix::Address.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 
>   3rdparty/libprocess/include/process/address.hpp 04e3155d65f476208fa852e83b79d173b66288fd

>   3rdparty/libprocess/include/process/network.hpp 52110667185370a4c92e2fa524819ab1f34bdec9

>   3rdparty/libprocess/include/process/socket.hpp f798af7879546d71e8ef4a295c9cf489a70cb61f

>   3rdparty/libprocess/include/process/ssl/gtest.hpp 21a0fc45b55a368a21b3e616c751ab43eebd4902

>   3rdparty/libprocess/src/address.cpp PRE-CREATION 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 5c0929d3d9f5595bd2f343b98b899fd6b06a67b2

>   3rdparty/libprocess/src/poll_socket.cpp eb7b48713edd30b545d7be95b5d51b0f71bd422a 
>   3rdparty/libprocess/src/process.cpp e9a4bbb0b2410e0260d120b97e73972c94eb0f26 
>   3rdparty/libprocess/src/socket.cpp 7f93168e1572f8669f67a4c5e6e5467259b7a407 
>   3rdparty/libprocess/src/tests/socket_tests.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 55c8c309571b1892b0acc4d766eda9bb98085a6f

>   3rdparty/libprocess/src/tests/test_linkee.cpp 1f6cfafcb73fd41ef350b13e3ac6023d78f16f5a

> 
> Diff: https://reviews.apache.org/r/53460/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>


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