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