----------------------------------------------------------- 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) <https://reviews.apache.org/r/64908/#comment274536> None is EOF? 3rdparty/libprocess/src/http_proxy.cpp Lines 57 (patched) <https://reviews.apache.org/r/64908/#comment274537> 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) <https://reviews.apache.org/r/64908/#comment274539> 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) <https://reviews.apache.org/r/64908/#comment274538> 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) <https://reviews.apache.org/r/64908/#comment274540> Should this take an Owned to clarify that the ownership is passed in? 3rdparty/libprocess/src/http_proxy.cpp Lines 339 (patched) <https://reviews.apache.org/r/64908/#comment274544> 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) <https://reviews.apache.org/r/64908/#comment274542> 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) <https://reviews.apache.org/r/64908/#comment274548> 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) <https://reviews.apache.org/r/64908/#comment274547> 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 > dd4d111c4ae579420060e547d1111d12f8f0711c > > > Diff: https://reviews.apache.org/r/64908/diff/2/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
