mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@mesosphere.io>
Subject Re: Review Request 62252: Added `process::Executor::execute()`.
Date Wed, 13 Sep 2017 15:02:37 GMT


> On Sept. 13, 2017, 5:10 a.m., Benjamin Hindman wrote:
> > 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.

Thanks for your comments! (1) makes a lot sense to me and I also don't like the current approach
of using macros. For (2), I don't think the way `dispatch()` taking care of `void` is what
we want, because `dispatch(pid, f)` returns `void` if `f` returns `void` and thus we cannot
wait on it. That's the intention of the `Executor::Process::execute` method: turning `void`
to `Nothing`. Am I missing something?


- Chun-Hung


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


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