mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 64908: Refactored `HttpProxy` to not rely on `SocketManager`.
Date Sat, 13 Jan 2018 01:38:08 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message