mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 58977: Add local and peer address accessors to http::Connection.
Date Fri, 12 May 2017 00:15:33 GMT

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




3rdparty/libprocess/include/process/http.hpp
Lines 960-968 (patched)
<https://reviews.apache.org/r/58977/#comment247965>

    Per our offline discussion, seems like we could avoid the 'Try's here?
    
    For `peer()`, `http::connect` takes the peer address to begin with, so we should just
be able to carry that through here without having to call `peer()` on the socket.
    
    For `address()`, see my suggestion below.



3rdparty/libprocess/include/process/http.hpp
Lines 963 (patched)
<https://reviews.apache.org/r/58977/#comment247967>

    Naming wise, I can forsee confusion when I call connection.address() and I get the local
rather than peer address, e.g. I see libraries where 'address' refers to the peer address:
    
    https://android.googlesource.com/platform/libcore/+/a0d32add4376f31f35e2a50f59185f16f5cd8ccb/luni/src/main/java/libcore/net/http/HttpConnection.java#181
    
    Also, our own http::connect function takes the peer address as just "address", so it seems
prone to confusion that Connection "address" is the local address and http::connect("address")
is the peer.
    
    How about localAddress and peerAddress to be explicit?



3rdparty/libprocess/include/process/http.hpp
Lines 968 (patched)
<https://reviews.apache.org/r/58977/#comment247968>

    Can these functions be const?



3rdparty/libprocess/src/http.cpp
Lines 1407-1410 (original), 1423-1426 (patched)
<https://reviews.apache.org/r/58977/#comment247966>

    To deal with the case of `socket->address()` failing, this could become:
    
    ```
    Try<Address> local_address = socket->address();
    if (local_address.isError()) {
      return Failure("Failed to get socket's address: " + local_address.error());
    }
    
    return socket->connect(peer_address)
      .then([socket, local_address, peer_addresss]() {
        return Connection(socket.get(), local_address.geT(), peer_address);
      }
    ```


- Benjamin Mahler


On May 10, 2017, 6:05 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58977/
> -----------------------------------------------------------
> 
> (Updated May 10, 2017, 6:05 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7401
>     https://issues.apache.org/jira/browse/MESOS-7401
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add local and peer address accessors to http::Connection.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/http.hpp 650b9d8aaba5636e1a445abf9e27e018ff82e610

>   3rdparty/libprocess/src/http.cpp 9789607933745f1fc4e37f47ce1be6aecb33a6e6 
> 
> 
> Diff: https://reviews.apache.org/r/58977/diff/3/
> 
> 
> Testing
> -------
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>


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