Re: Review Request 62252: Added `process::Executor::execute()`.

2017-09-25 Thread Chun-Hung Hsiao 

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

(Updated Sept. 25, 2017, 8:48 p.m.)


Review request for mesos, Benjamin Hindman, Benjamin Mahler, and Michael Park.


Changes
---

Addressed benh's comments and rebased.


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/7/

Changes: https://reviews.apache.org/r/62252/diff/6-7/


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 62252: Added `process::Executor::execute()`.

2017-09-25 Thread Benjamin Hindman 

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


Fix it, then Ship it!




Just a few minor cleanups and then I'll commit it.


3rdparty/libprocess/src/tests/process_tests.cpp
Line 1212 (original), 1215 (patched)


Let's use lambdas! I've seen you do this in previous patches and it's just 
easier to read (than to have to specify the member function):

```
.WillOnce(InvokeWithoutArgs([&]() {
  event1Called.decrement();
}));
```



3rdparty/libprocess/src/tests/process_tests.cpp
Lines 1214-1215 (original), 1217-1218 (patched)


As long as you're moving stuff around, let's move this and the second 
`CountDownLatch` down to after the first `AWAIT_READY` too.



3rdparty/libprocess/src/tests/process_tests.cpp
Lines 1252 (patched)


`AWAIT_READY(executor.execute(f1));`



3rdparty/libprocess/src/tests/process_tests.cpp
Lines 1264 (patched)


`AWAIT_READY(executor.execute(std::bind(f2, 42)));`



3rdparty/libprocess/src/tests/process_tests.cpp
Lines 1274-1276 (patched)


We can simplify these next ones: `AWAIT_EXPECT_EQ(f3Result, 
executor.execute(f3));`



3rdparty/libprocess/src/tests/process_tests.cpp
Lines 1286-1288 (patched)


`AWAIT_EXPECT_EQ(f4Result, executor.execute(std::bind(f4, 42)));`


- Benjamin Hindman


On Sept. 21, 2017, 8:53 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62252/
> ---
> 
> (Updated Sept. 21, 2017, 8:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Benjamin Mahler, and Michael Park.
> 
> 
> 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/6/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 62252: Added `process::Executor::execute()`.

2017-09-21 Thread Chun-Hung Hsiao 

---
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()`.

2017-09-20 Thread Benjamin Hindman 

---
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)


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)


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));`



3rdparty/libprocess/src/tests/process_tests.cpp
Lines 1242 (patched)


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()`.

2017-09-14 Thread Chun-Hung Hsiao 

---
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()`.

2017-09-14 Thread Benjamin Hindman 

---
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)


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`. Let's add a test for this case too please!



3rdparty/libprocess/include/process/executor.hpp
Lines 63 (patched)


Let's still take a universal reference for `F`, e.g., `F&&`. Then we can 
`std::forward(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)


Can we comment that this overload for `void` returns `Future` 
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)


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` inside the void function instead 
of a `std::atomic`? 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()`.

2017-09-13 Thread Chun-Hung Hsiao 

---
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()`.

2017-09-13 Thread Chun-Hung Hsiao 


> 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()`.

2017-09-12 Thread Benjamin Hindman 

---
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()`.

2017-09-12 Thread Chun-Hung Hsiao 

---
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

Review Request 62252: Added `process::Executor::execute()`.

2017-09-12 Thread Chun-Hung Hsiao 

---
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