You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2017/05/03 22:35:13 UTC
Review Request 58977: Add local and peer address accessors to
http::Connection.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58977/
-----------------------------------------------------------
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/1/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 58977: Add local and peer address accessors to
http::Connection.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58977/#review176342
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Mahler
On May 17, 2017, 4:58 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58977/
> -----------------------------------------------------------
>
> (Updated May 17, 2017, 4:58 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/5/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 58977: Add local and peer address accessors to
http::Connection.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58977/
-----------------------------------------------------------
(Updated May 17, 2017, 4:58 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Updated with review feedback.
Bugs: MESOS-7401
https://issues.apache.org/jira/browse/MESOS-7401
Repository: mesos
Description
-------
Add local and peer address accessors to http::Connection.
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp 650b9d8aaba5636e1a445abf9e27e018ff82e610
3rdparty/libprocess/src/http.cpp 9789607933745f1fc4e37f47ce1be6aecb33a6e6
Diff: https://reviews.apache.org/r/58977/diff/4/
Changes: https://reviews.apache.org/r/58977/diff/3-4/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 58977: Add local and peer address accessors to
http::Connection.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 58977: Add local and peer address accessors to
http::Connection.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
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 (updated)
-----
3rdparty/libprocess/include/process/http.hpp 650b9d8aaba5636e1a445abf9e27e018ff82e610
3rdparty/libprocess/src/http.cpp 9789607933745f1fc4e37f47ce1be6aecb33a6e6
Diff: https://reviews.apache.org/r/58977/diff/2/
Changes: https://reviews.apache.org/r/58977/diff/1-2/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 58977: Add local and peer address accessors to
http::Connection.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58977/#review174233
-----------------------------------------------------------
3rdparty/libprocess/include/process/http.hpp
Lines 960-968 (patched)
<https://reviews.apache.org/r/58977/#comment247345>
Hm.. is it possible to make these synchronous? Since these are synchronous accesses to the state, it seems unfortuante to be returning futures here, I can imagine this will be burdensome on the callers.
Is it possible at construction time of `Connection` to get this information out of the socket that is passed in and store it within `Data`? That way, we could just read them out synchronously here.
- Benjamin Mahler
On May 3, 2017, 10:35 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58977/
> -----------------------------------------------------------
>
> (Updated May 3, 2017, 10:35 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/1/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>