Re: Review Request 64908: Refactored `HttpProxy` to not rely on `SocketManager`.

[Benjamin Mahler]

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


Fix it, then Ship it!




Looks good, just looks like the comments at the bottom of the socket manager 
need to be updated to reflect the removal of the "inbound" socket tracking.


3rdparty/libprocess/src/http_proxy.hpp
Lines 86-88 (patched)


None is EOF?



3rdparty/libprocess/src/http_proxy.cpp
Lines 57 (patched)


It took me awhile to figure out where this was coming from, wonder if it 
should be within an `encoder` namespace.



3rdparty/libprocess/src/http_proxy.cpp
Lines 63-67 (patched)


I wonder if we should log the failure case here, although not sure if we 
have a sufficient message in the future.



3rdparty/libprocess/src/http_proxy.cpp
Lines 75-85 (patched)


We can do this later, but just FYI this message has thrown off users for 
some time, in the case that it comes out from a socket already being closed. I 
wish we could check for that case and ignore it. We already have `SocketError`, 
would just need to wire it through.



3rdparty/libprocess/src/http_proxy.cpp
Lines 330 (patched)


Should this take an Owned to clarify that the ownership is passed in?



3rdparty/libprocess/src/http_proxy.cpp
Lines 339 (patched)


Can we move the response in? Looks like we copy one unnecessary time from 
the caller into the encoder here?



3rdparty/libprocess/src/http_proxy.cpp
Lines 346 (patched)


You can use .at here if you like, although what I don't like about it is it 
throws an exception rather than aborting:

```
if (response.headers.at("Connection") == "close") {
```



3rdparty/libprocess/src/process.cpp
Lines 1268-1274 (original), 1274-1279 (patched)


Huh.. I guess the "inbound" sockets get closed now via the HttpProxies 
being terminated above in the process manager finalization, nice.



3rdparty/libprocess/src/socket_manager.hpp
Lines 124-147 (original), 100-123 (patched)


My understanding here is that before we stored both "inbound" (client 
connected to our server) and "outbound" (we connected to the client's server) 
scokets, and the comments here reflect that.

Now, we only store the "outbound" case, so it seems like these comments 
need an update to reflect that?


- Benjamin Mahler


On Jan. 3, 2018, 7:20 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64908/
> ---
> 
> (Updated Jan. 3, 2018, 7:20 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This simplifies the introduction of `http::Server` so that it's easier
> to reason about and in the future will make removing the `HttpProxy`
> implementation much easier.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/http_proxy.hpp 
> 5b6e7e8786ed9eab50cd4c2cfdec455c92d72eca 
>   3rdparty/libprocess/src/http_proxy.cpp 
> f584238aadd86875d7c87736653f394e716397de 
>   3rdparty/libprocess/src/process.cpp 
> 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf 
>   3rdparty/libprocess/src/socket_manager.hpp 
> dd4d111c4ae579420060e547dd12f8f0711c 
> 
> 
> Diff: https://reviews.apache.org/r/64908/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Review Request 64908: Refactored `HttpProxy` to not rely on `SocketManager`.

[Benjamin Hindman]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

This simplifies the introduction of `http::Server` so that it's easier
to reason about and in the future will make removing the `HttpProxy`
implementation much easier.


Diffs
-

  3rdparty/libprocess/src/http_proxy.hpp 
5b6e7e8786ed9eab50cd4c2cfdec455c92d72eca 
  3rdparty/libprocess/src/http_proxy.cpp 
f584238aadd86875d7c87736653f394e716397de 
  3rdparty/libprocess/src/process.cpp 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf 
  3rdparty/libprocess/src/socket_manager.hpp 
dd4d111c4ae579420060e547dd12f8f0711c 


Diff: https://reviews.apache.org/r/64908/diff/1/


Testing
---

make check


Thanks,

Benjamin Hindman