mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Hindman <b...@berkeley.edu>
Subject Re: Review Request 62252: Added `process::Executor::execute()`.
Date Wed, 13 Sep 2017 05:10:38 GMT

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



Thanks for taking this on Chun! A few high level comments to start.

(1) I don't think we need to implement a version of `execute()` that takes arguments that
we'll apply to the function. With lambda captures we can easily and cleanly accomplish that
and any of the corner cases will be cleanly solved for with C++14 which we should move too
sooner rather than later. If folks really want that functionality I'd rather just see them
use `std::bind()` and do `executor.execute(std::bind(f, arg1, arg2))`. It's not that many
more characters and it greately simplifies your implementation!

(2) Given the simplification of (1) I feel like you probably don't need a separate `Executor::Process`
class, and instead can just use `dispatch()` on the existing `process`. You should be able
to simplify your SFINAE by leveraging the return type `dispatch()` since it already takes
care of the `void` issue for you.

- Benjamin Hindman


On Sept. 12, 2017, 8:12 p.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62252/
> -----------------------------------------------------------
> 
> (Updated Sept. 12, 2017, 8:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7970
>     https://issues.apache.org/jira/browse/MESOS-7970
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch adds a convenient interface to `process::Executor` to
> asynchronously execute arbitrary functions.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/executor.hpp cd2f2f03cba8a435f142b31def9f89a6cd61af73

>   3rdparty/libprocess/src/tests/process_tests.cpp 82efb2f8449e4cb13620cae1a49321fc42e2db60

> 
> 
> Diff: https://reviews.apache.org/r/62252/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


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