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 Fri, 11 Nov 2016 21:27:25 GMT

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



Looking good! Just a few minor things left.


3rdparty/libprocess/src/process.cpp (lines 680 - 681)
<https://reviews.apache.org/r/53487/#comment225821>

    Don't you need to change the type to BODY here as well?



3rdparty/libprocess/src/process.cpp (lines 708 - 722)
<https://reviews.apache.org/r/53487/#comment225824>

    Now that we're returning Future<Message*> we can capture failures explicitly as
a failed future, so that the caller doesn't have to consider two failure cases. In other words,
if this returns a successful future, the pointer will be non-null. 
    
    (See my comment below for how that simplifies the call-site).



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

    It looks like this may be introducing another problematic path for libprocess finalization,
in that your continuation may run after libprocess is finalized and it expects 'this' to be
valid.
    
    It would be great to check with Joesph Wu about this path being introduced since he has
worked on libprocess finalization, and see if he has any input.



3rdparty/libprocess/src/process.cpp (lines 2732 - 2743)
<https://reviews.apache.org/r/53487/#comment225823>

    The intention of my previous suggestion to return Future<Message*> was to eliminate
the possibility of nullptr in favor of just returning a failed future for this case.
    
    Then you only need to deal with one error path here and you can just do the following:
    
    ```
    Message* message = CHECK_NOTNULL(future.get());
    ```



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

    TODO to avoid copying the response?
    
    Also, mind using associate? We have set as an alias but I believe most code calls associate
for associating with another future since it's a bit more specific.



3rdparty/libprocess/src/process.cpp (lines 3683 - 3684)
<https://reviews.apache.org/r/53487/#comment225826>

    It seems problematic to delay the association until the response transitions out of pending,
as this will prevent discard propagation. (e.g. the socket disconnects and libprocess discards
the response, the discard won't propagate into the handler).
    
    Can you instead perform the association as soon as you've invoked the handlers, before
waiting for the response to transition?


- Benjamin Mahler


On Nov. 11, 2016, 7:57 p.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53487/
> -----------------------------------------------------------
> 
> (Updated Nov. 11, 2016, 7:57 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.
> 
> 
> 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