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 53487: Wired the libprocess code to use the streaming decoder.
Date Mon, 21 Nov 2016 20:36:24 GMT

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


Fix it, then Ship it!




The structure of visit looks better now, thanks!


3rdparty/libprocess/src/process.cpp (line 672)
<https://reviews.apache.org/r/53487/#comment226674>

    `const Owned&` seems to suggest that this doesn't take ownership, perhaps take `Owned`?



3rdparty/libprocess/src/process.cpp (lines 2712 - 2713)
<https://reviews.apache.org/r/53487/#comment226675>

    Can you clarify for the reader why socket manager finalization is tied to this continuation
being invoked?



3rdparty/libprocess/src/process.cpp (lines 2716 - 2717)
<https://reviews.apache.org/r/53487/#comment226677>

    Hm.. seems this is where we need a comment about how we're guaranteed that the socket
manager is valid inside this continuation?



3rdparty/libprocess/src/process.cpp (lines 2725 - 2726)
<https://reviews.apache.org/r/53487/#comment226676>

    Could you include the open and closing quotes on the same line? Also, any reason not to
use the typical ": " based approach for logging the error?
    
    ```
              VLOG(1) << "Returning '" << response.status << "'"
                      << " for '" << request->url.path << "'"
                      << ": " << response.body << "";
    ```



3rdparty/libprocess/src/process.cpp (line 3617)
<https://reviews.apache.org/r/53487/#comment226678>

    Why the copy? Do you want a const reference here?



3rdparty/libprocess/src/process.cpp (lines 3648 - 3673)
<https://reviews.apache.org/r/53487/#comment226680>

    Can you add a TODO (in a separate patch) that we should handle discarded responses by
not processing the request?
    
    As it stands, if the event is in the queue and the caller disconnects, we will still process
the request here and deliver it to the route.



3rdparty/libprocess/src/process.cpp (line 3665)
<https://reviews.apache.org/r/53487/#comment226681>

    No need for "due to" here? Seems inconsistent with our other error logging style.



3rdparty/libprocess/src/process.cpp (line 3669)
<https://reviews.apache.org/r/53487/#comment226682>

    No need for the return.



3rdparty/libprocess/src/process.cpp (lines 3676 - 3677)
<https://reviews.apache.org/r/53487/#comment226683>

    Maybe a TODO like:
    
    ```
    TODO(anandm): Is this an error?
    ```
    
    Since I'm not sure if we should silently allow it.



3rdparty/libprocess/src/process.cpp (line 3705)
<https://reviews.apache.org/r/53487/#comment226685>

    Not your change, but using associate here seems confusing, since it is intended for association
with another Future. It happens to compile because it is implicitly constructs a `Future<Response>`
from the `Response`. So:
    
    ```
    event.response->set(response);
    ```
    
    Would be clearer. Feel free to make this cleanup (and the other s/associate/set/ suggestion)
in a patch pulled out from this one.



3rdparty/libprocess/src/process.cpp (line 3713)
<https://reviews.apache.org/r/53487/#comment226684>

    Using associate here seems a little odd, since it is intended for association with another
Future. It compiles because it is implicitly constructure a `Future<Response>` from
`NotFound()`. So:
    
    ```
    event.response->set(NotFound());
    ```
    
    Would be clearer. Feel free to make this cleanup (and the other s/associate/set/ suggestion)
in a patch pulled out from this one.



3rdparty/libprocess/src/process.cpp (line 3720)
<https://reviews.apache.org/r/53487/#comment226679>

    Seems to indicate that the ownership resides in the caller, as if you did:
    
    ```
    void function(const unique_ptr& p)
    ```
    
    Whereas the following would seem to suggest passing ownership:
    
    ```
    void function(unique_ptr&& p)
    void function(unique_ptr p)
    ```
    
    Also, in these latter options, you can move the pointer into a capture (not yet in C++11),
whereas you couldn't with the first option.


- Benjamin Mahler


On Nov. 21, 2016, 7:25 p.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53487/
> -----------------------------------------------------------
> 
> (Updated Nov. 21, 2016, 7:25 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6466
>     https://issues.apache.org/jira/browse/MESOS-6466
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Old libprocess style messages and routes not supporting request
> streaming read the body from the piped reader. Otherwise, the
> request is forwarded to the handler when the route supports
> streaming.
> 
> Review: https://reviews.apache.org/r/53487/
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/process.hpp f389d3d3b671e301a7ac911ad87ab13289e8c82f

>   3rdparty/libprocess/src/process.cpp ab2b5a9d38a3001d6a5daa1807fecb630c4b154d 
> 
> Diff: https://reviews.apache.org/r/53487/diff/
> 
> 
> Testing
> -------
> 
> make check (Tests are added in https://reviews.apache.org/r/53490 i.e., after we add
support to the `Connection` abstraction for request streaming)
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>


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