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 54110: Added `process::loop` abstraction.
Date Tue, 29 Nov 2016 02:58:46 GMT

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


Fix it, then Ship it!




There are also some comments in your pending review (e.g. re-structuring `run()` to always
call `iterate()` at the beginning). Otherwise, looks good.

Will be interesting to see how we can integrate this into the rest of our code. I've wanted
an asynchronous loop construct for awhile, we've written a lot of "delay loops" that have
complicated cancellation handling.


3rdparty/libprocess/include/process/loop.hpp (lines 44 - 47)
<https://reviews.apache.org/r/54110/#comment227626>

    Can you update all of these examples to show that body is also an asynchronous function?



3rdparty/libprocess/include/process/loop.hpp (lines 98 - 111)
<https://reviews.apache.org/r/54110/#comment227642>

    Can we make this a managed process so that we don't have this wait issue?
    
    See this example:
    https://github.com/apache/mesos/blob/58f63747f185995d7f9cbfca9d240e2d60053184/3rdparty/libprocess/src/http.cpp#L1253-L1274



3rdparty/libprocess/include/process/loop.hpp (lines 117 - 131)
<https://reviews.apache.org/r/54110/#comment227641>

    Maybe we make this a class with methods?
    
    ```
    class Loop
    {
    public:
      void start();
      void run();
      // more?
    };
    ```
    
    To make the lifecycle more clear?



3rdparty/libprocess/include/process/loop.hpp (line 129)
<https://reviews.apache.org/r/54110/#comment227643>

    Maybe '`item`'?



3rdparty/libprocess/include/process/loop.hpp (lines 212 - 228)
<https://reviews.apache.org/r/54110/#comment227644>

    Once this moves into Loop::start / Loop::initialize / whatever we want to call it, then
perhaps we can document the overall discard strategy at the top of `Loop`?



3rdparty/libprocess/src/tests/loop_tests.cpp (lines 28 - 75)
<https://reviews.apache.org/r/54110/#comment227628>

    Can you also add a test for discard semantics?


- Benjamin Mahler


On Nov. 28, 2016, 6:33 p.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54110/
> -----------------------------------------------------------
> 
> (Updated Nov. 28, 2016, 6:33 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added `process::loop` abstraction.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 71319891a451bd1d565210dcce2fb61fc69e1f61 
>   3rdparty/libprocess/include/process/loop.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/loop_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54110/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>


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