----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61158/#review191977 -----------------------------------------------------------
I'm quite concerned about maintaining the two code paths for the socket manager, and I couldn't quite figure out the code. For example, where does the `sockets` map get written to in the case of the http server? 3rdparty/libprocess/src/http_proxy.hpp Lines 13 (patched) <https://reviews.apache.org/r/61158/#comment269953> Hm.. why did you do this? 3rdparty/libprocess/src/process.cpp Lines 463-480 (original), 465-488 (patched) <https://reviews.apache.org/r/61158/#comment269956> Using an #else seems a little cleaner than the two seperate #ifndef/#ifdef sections? 3rdparty/libprocess/src/process.cpp Lines 1137-1147 (original), 1149-1165 (patched) <https://reviews.apache.org/r/61158/#comment269959> This seems a little odd, I would expect the http::Server and HttpProxy setup here to be in the same location with respect to the rest of initialization (e.g. using #idef/#else as well), is there something preventing that? Or something requiring them to be this far apart? 3rdparty/libprocess/src/process.cpp Lines 1235-1273 (patched) <https://reviews.apache.org/r/61158/#comment269961> Again, puzzling to find this in a different part of the initialization than the HttpProxy case. 3rdparty/libprocess/src/process.cpp Lines 1243 (patched) <https://reviews.apache.org/r/61158/#comment269970> Do you need all this prefixing? Or will SocketImpl::Kind::SSL or shorter be resolved? 3rdparty/libprocess/src/process.cpp Lines 1250-1253 (patched) <https://reviews.apache.org/r/61158/#comment269972> The capture by reference scared me, then I realized that nothing is getting captured by reference since process_manager is a global? Can you remove the `&`? 3rdparty/libprocess/src/process.cpp Lines 1261 (patched) <https://reviews.apache.org/r/61158/#comment269980> Would be helpful to give some context, like we tried to do for some of the other fatal logging: ``` LOG(FATAL) << "Failed to initialize, failed to construct http server: " << server.error(); ``` 3rdparty/libprocess/src/process.cpp Lines 1313 (patched) <https://reviews.apache.org/r/61158/#comment269981> s/shutdown/stop/ - Benjamin Mahler On Nov. 26, 2017, 10:28 p.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61158/ > ----------------------------------------------------------- > > (Updated Nov. 26, 2017, 10:28 p.m.) > > > Review request for mesos, Alexander Rukletsov, Avinash sridharan, and > Benjamin Mahler. > > > Repository: mesos > > > Description > ------- > > Using http::Server can be compile time configured via USE_HTTP_SERVER. > > > Diffs > ----- > > 3rdparty/libprocess/src/http_proxy.hpp > 5b6e7e8786ed9eab50cd4c2cfdec455c92d72eca > 3rdparty/libprocess/src/http_proxy.cpp > f584238aadd86875d7c87736653f394e716397de > 3rdparty/libprocess/src/process.cpp > 64bcce215d19558dd493e30e96ca16577fe0722a > 3rdparty/libprocess/src/socket_manager.hpp > dd4d111c4ae579420060e547d1111d12f8f0711c > > > Diff: https://reviews.apache.org/r/61158/diff/2/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
