You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@mesosphere.io> on 2017/09/12 18:12:41 UTC
Review Request 62252: Added `process::Executor::execute()`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62252/
-----------------------------------------------------------
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/1/
Testing
-------
make check
Thanks,
Chun-Hung Hsiao
Re: Review Request 62252: Added `process::Executor::execute()`.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62252/#review185814
-----------------------------------------------------------
3rdparty/libprocess/include/process/executor.hpp
Lines 71 (patched)
<https://reviews.apache.org/r/62252/#comment262158>
Let's use the `std::enable_if<..., int>::type = 0` for now since that's what we've been trying to converge on other places. Here and below, thanks!
3rdparty/libprocess/include/process/executor.hpp
Lines 87 (patched)
<https://reviews.apache.org/r/62252/#comment262159>
Can we also add a TODO that we ultimately want to capture `f` via a forward/move? Thanks!
Can we do this?
`std::bind([](F&& f) { f(); return Nothing(); }, std::forward<F>(f));`
3rdparty/libprocess/src/tests/process_tests.cpp
Lines 1242 (patched)
<https://reviews.apache.org/r/62252/#comment262161>
Can we update the code to use `CountDownLatch` so we can use `AWAIT_READY(latch.triggered())` and also pull all the test chunks together "locally" so that it's easier to see what each piece does. Thanks!
- Benjamin Hindman
On Sept. 15, 2017, 3:17 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62252/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2017, 3:17 a.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/5/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62252: Added `process::Executor::execute()`.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62252/
-----------------------------------------------------------
(Updated Sept. 21, 2017, 8:51 p.m.)
Review request for mesos, Benjamin Hindman and Benjamin Mahler.
Changes
-------
Addressed benh's comments. Made the API work with mutable lambdas and added a test for that.
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 (updated)
-----
3rdparty/libprocess/include/process/executor.hpp cd2f2f03cba8a435f142b31def9f89a6cd61af73
3rdparty/libprocess/src/tests/process_tests.cpp 82efb2f8449e4cb13620cae1a49321fc42e2db60
Diff: https://reviews.apache.org/r/62252/diff/6/
Changes: https://reviews.apache.org/r/62252/diff/5-6/
Testing
-------
make check
Thanks,
Chun-Hung Hsiao
Re: Review Request 62252: Added `process::Executor::execute()`.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62252/
-----------------------------------------------------------
(Updated Sept. 15, 2017, 3:17 a.m.)
Review request for mesos, Benjamin Hindman and Benjamin Mahler.
Changes
-------
Addressed Ben's comments.
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 (updated)
-----
3rdparty/libprocess/include/process/executor.hpp cd2f2f03cba8a435f142b31def9f89a6cd61af73
3rdparty/libprocess/src/tests/process_tests.cpp 82efb2f8449e4cb13620cae1a49321fc42e2db60
Diff: https://reviews.apache.org/r/62252/diff/4/
Changes: https://reviews.apache.org/r/62252/diff/3-4/
Testing
-------
make check
Thanks,
Chun-Hung Hsiao
Re: Review Request 62252: Added `process::Executor::execute()`.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62252/#review185464
-----------------------------------------------------------
3rdparty/libprocess/include/process/executor.hpp
Lines 62 (patched)
<https://reviews.apache.org/r/62252/#comment261762>
What about a function that returns a `Future` all ready? I think we should flatten the result on behalf of the user, rather than getting back a `Future<Future<string>>`. Let's add a test for this case too please!
3rdparty/libprocess/include/process/executor.hpp
Lines 63 (patched)
<https://reviews.apache.org/r/62252/#comment261761>
Let's still take a universal reference for `F`, e.g., `F&&`. Then we can `std::forward<F>(f)` to `dispatch()`. Same below even though we make a copy, eventually we'll be able to forward it as a lambda capture.
3rdparty/libprocess/include/process/executor.hpp
Lines 68-75 (patched)
<https://reviews.apache.org/r/62252/#comment261763>
Can we comment that this overload for `void` returns `Future<Nothing>` specifically so we can chain? And that it follows the `async()` pattern? It'll be great for future readers (including ourselves!) to understand (or rememeber) why we made this design decision.
3rdparty/libprocess/src/tests/process_tests.cpp
Lines 1238 (patched)
<https://reviews.apache.org/r/62252/#comment261764>
For the non-void returning function it seems unnecessary to also check `f2Result`. I'd suggest we simplify the test so nobody is guessing whether or not that part is really testing anything.
And what about using a `Promise<Nothing>` inside the void function instead of a `std::atomic<int>`? Or even a `Latch`?
And let's add a test for a function/lambda that already returns a `Future`.
- Benjamin Hindman
On Sept. 13, 2017, 5:34 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62252/
> -----------------------------------------------------------
>
> (Updated Sept. 13, 2017, 5:34 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/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 62252: Added `process::Executor::execute()`.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62252/
-----------------------------------------------------------
(Updated Sept. 13, 2017, 5:34 p.m.)
Review request for mesos, Benjamin Hindman and Benjamin Mahler.
Changes
-------
Addressed Ben's comments.
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 (updated)
-----
3rdparty/libprocess/include/process/executor.hpp cd2f2f03cba8a435f142b31def9f89a6cd61af73
3rdparty/libprocess/src/tests/process_tests.cpp 82efb2f8449e4cb13620cae1a49321fc42e2db60
Diff: https://reviews.apache.org/r/62252/diff/3/
Changes: https://reviews.apache.org/r/62252/diff/2-3/
Testing
-------
make check
Thanks,
Chun-Hung Hsiao
Re: Review Request 62252: Added `process::Executor::execute()`.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> 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
>
>
Re: Review Request 62252: Added `process::Executor::execute()`.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
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
>
>
Re: Review Request 62252: Added `process::Executor::execute()`.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
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.
Changes
-------
Moved `ExecutorProcess` to `Executor::Process` to avoid naming conflicts in Mesos.
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 (updated)
-----
3rdparty/libprocess/include/process/executor.hpp cd2f2f03cba8a435f142b31def9f89a6cd61af73
3rdparty/libprocess/src/tests/process_tests.cpp 82efb2f8449e4cb13620cae1a49321fc42e2db60
Diff: https://reviews.apache.org/r/62252/diff/2/
Changes: https://reviews.apache.org/r/62252/diff/1-2/
Testing
-------
make check
Thanks,
Chun-Hung Hsiao