You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2015/01/02 05:46:17 UTC
Review Request 29535: Added Future::repair.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29535/
-----------------------------------------------------------
Review request for mesos and Adam B.
Repository: mesos-git
Description
-------
See summary.
Diffs
-----
3rdparty/libprocess/include/process/future.hpp 0326b23cdd475c6e86b33f9b4c63136fdecab443
3rdparty/libprocess/src/tests/process_tests.cpp 3bbfe0a7a65acb52d139fda81816acf305d891f5
Diff: https://reviews.apache.org/r/29535/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 29535: Added Future::repair.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29535/#review68983
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/src/tests/process_tests.cpp
<https://reviews.apache.org/r/29535/#comment113550>
You could be more explicit in commenting how you're testing that the 'repair' callback doesn't get executed. I'm assuming it's because you actually get back the 42 you set, rather than the 43 from repair, but it's not super obvious.
- Adam B
On Jan. 21, 2015, 9:47 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29535/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2015, 9:47 a.m.)
>
>
> Review request for mesos and Adam B.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp 0326b23cdd475c6e86b33f9b4c63136fdecab443
> 3rdparty/libprocess/src/tests/process_tests.cpp 3bbfe0a7a65acb52d139fda81816acf305d891f5
>
> Diff: https://reviews.apache.org/r/29535/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 29535: Added Future::repair.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29535/#review69308
-----------------------------------------------------------
3rdparty/libprocess/include/process/future.hpp
<https://reviews.apache.org/r/29535/#comment113960>
My impression is that repair is meant to be used liberally to deal with error recover, and makes the use of futures much easier to read.
Under the assumption that errors / exceptions are rare, it seems unfortunate to force this extra callback in the chain even for successful futures. This will degrade performance of healthy systems.
Can we refactor this such that the callback is only called when we are actually in a failure scenario?
- Joris Van Remoortere
On Jan. 21, 2015, 5:47 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29535/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2015, 5:47 p.m.)
>
>
> Review request for mesos and Adam B.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp 0326b23cdd475c6e86b33f9b4c63136fdecab443
> 3rdparty/libprocess/src/tests/process_tests.cpp 3bbfe0a7a65acb52d139fda81816acf305d891f5
>
> Diff: https://reviews.apache.org/r/29535/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 29535: Added Future::repair.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29535/
-----------------------------------------------------------
(Updated Jan. 21, 2015, 5:47 p.m.)
Review request for mesos and Adam B.
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/include/process/future.hpp 0326b23cdd475c6e86b33f9b4c63136fdecab443
3rdparty/libprocess/src/tests/process_tests.cpp 3bbfe0a7a65acb52d139fda81816acf305d891f5
Diff: https://reviews.apache.org/r/29535/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 29535: Added Future::repair.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Jan. 5, 2015, 7:23 a.m., Adam B wrote:
> > 3rdparty/libprocess/include/process/future.hpp, lines 1499-1500
> > <https://reviews.apache.org/r/29535/diff/1/?file=805390#file805390line1499>
> >
> > Any reason why internal::repair places the lambda parameter before the promise, and internal::thenf reverses the order?
Made them all be consistent. For posterity, I've been making the continuations take the arguments that the previous function had first, followed by any new arguments created. This should make the conversions to lambda even easier.
> On Jan. 5, 2015, 7:23 a.m., Adam B wrote:
> > 3rdparty/libprocess/include/process/future.hpp, lines 1597-1600
> > <https://reviews.apache.org/r/29535/diff/1/?file=805390#file805390line1597>
> >
> > Maybe worth making a private helper function here, since it's already repeated so many times.
But it's small and an explicit bit of code, I'd hate to lose that in a helper that people don't remember what it does.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29535/#review66600
-----------------------------------------------------------
On Jan. 21, 2015, 5:47 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29535/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2015, 5:47 p.m.)
>
>
> Review request for mesos and Adam B.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp 0326b23cdd475c6e86b33f9b4c63136fdecab443
> 3rdparty/libprocess/src/tests/process_tests.cpp 3bbfe0a7a65acb52d139fda81816acf305d891f5
>
> Diff: https://reviews.apache.org/r/29535/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 29535: Added Future::repair.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29535/#review66600
-----------------------------------------------------------
Ship it!
LGTM, but you could add simple unit tests for (not) repairing discarded/ready futures.
3rdparty/libprocess/include/process/future.hpp
<https://reviews.apache.org/r/29535/#comment110181>
Unrelated, but appreciated documentation update. :)
3rdparty/libprocess/include/process/future.hpp
<https://reviews.apache.org/r/29535/#comment110184>
Any reason why internal::repair places the lambda parameter before the promise, and internal::thenf reverses the order?
3rdparty/libprocess/include/process/future.hpp
<https://reviews.apache.org/r/29535/#comment110185>
Maybe worth making a private helper function here, since it's already repeated so many times.
3rdparty/libprocess/src/tests/process_tests.cpp
<https://reviews.apache.org/r/29535/#comment110180>
Shouldn't you also test that the repair callback is skipped if the future succeeds or is discarded?
- Adam B
On Jan. 1, 2015, 8:46 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29535/
> -----------------------------------------------------------
>
> (Updated Jan. 1, 2015, 8:46 p.m.)
>
>
> Review request for mesos and Adam B.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp 0326b23cdd475c6e86b33f9b4c63136fdecab443
> 3rdparty/libprocess/src/tests/process_tests.cpp 3bbfe0a7a65acb52d139fda81816acf305d891f5
>
> Diff: https://reviews.apache.org/r/29535/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 29535: Added Future::repair.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29535/#review66754
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/include/process/future.hpp
<https://reviews.apache.org/r/29535/#comment110366>
This is great! Here are just some thoughts on how should we use this primitive.
In many cases, we want to do async operation chaining. For example:
```
Future f = async1()
.then(async2)
.then(async3)
....
```
If any of the async operations returns error in the above chain, the chain will short cut and return a failure. Sometimes, however, we want to execute all the async operations even if some of them have failures (probably different behavior in the case of failure). So we probably would do something like the following using this primitive:
```
Future f = async1()
.repair(r1) // r1 returns a Future<Future<X>> so that async2 can use the passed-in Future<X> to decide it's action.
.then(async2)
.repair(r2) // Ditto.
.then(...)
...
```
The only unfortunate part here is that r1 needs to return Future<Future<X>> which is a little unintuititve.
- Jie Yu
On Jan. 2, 2015, 4:46 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29535/
> -----------------------------------------------------------
>
> (Updated Jan. 2, 2015, 4:46 a.m.)
>
>
> Review request for mesos and Adam B.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/future.hpp 0326b23cdd475c6e86b33f9b4c63136fdecab443
> 3rdparty/libprocess/src/tests/process_tests.cpp 3bbfe0a7a65acb52d139fda81816acf305d891f5
>
> Diff: https://reviews.apache.org/r/29535/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>