Re: Review Request 61158: Introduced http::Server in process.cpp.

[Benjamin Mahler]

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


Ship it!




Can we clarify why we ifdef guarded it? Specifically only to ensure that 
there's no performance regression before removing the HttpProxy, right?


3rdparty/libprocess/src/process.cpp
Lines 1239 (patched)


s/incomping/incoming/



3rdparty/libprocess/src/process.cpp
Lines 1244 (patched)


CHECK_EQ?



3rdparty/libprocess/src/process.cpp
Lines 1262 (patched)


http server?


- 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/process.cpp 
> 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf 
> 
> 
> Diff: https://reviews.apache.org/r/61158/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 61158: Introduced http::Server in process.cpp.

[Benjamin Mahler]

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


Hm.. why did you do this?



3rdparty/libprocess/src/process.cpp
Lines 463-480 (original), 465-488 (patched)


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)


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)


Again, puzzling to find this in a different part of the initialization than 
the HttpProxy case.



3rdparty/libprocess/src/process.cpp
Lines 1243 (patched)


Do you need all this prefixing? Or will SocketImpl::Kind::SSL or shorter be 
resolved?



3rdparty/libprocess/src/process.cpp
Lines 1250-1253 (patched)


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)


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)


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 
> dd4d111c4ae579420060e547dd12f8f0711c 
> 
> 
> Diff: https://reviews.apache.org/r/61158/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Review Request 61158: Introduced http::Server in process.cpp.

[Benjamin Hindman]

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

Review request for mesos, 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 
dd4d111c4ae579420060e547dd12f8f0711c 


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


Testing
---

make check


Thanks,

Benjamin Hindman