You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2014/01/30 06:42:13 UTC
Re: Review Request 17476: Added a Sequence abstraction.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17476/
-----------------------------------------------------------
(Updated Jan. 30, 2014, 5:42 a.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Changed the summary.
Summary (updated)
-----------------
Added a Sequence abstraction.
Repository: mesos-git
Description
-------
See summary.
One think I haven't done yet is to make it accepts '_Defer' types. (Just curious why Future.onAny can accept defer without doing something special).
Also, we can get rid of the SequencerProcess if we use a mutex to protect 'last'. Not sure which one is better.
The discard semantics here is a bit tricky. Basically, I wanna support discarding a single callback without affecting other callbacks. Also, when the Sequencer object is deleted, I wanna discard all the pending callbacks.
Diffs
-----
3rdparty/libprocess/Makefile.am 40f01a7
3rdparty/libprocess/include/process/sequence.hpp PRE-CREATION
3rdparty/libprocess/src/tests/sequence_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/17476/diff/
Testing
-------
make check
repeated 1000 times
Thanks,
Jie Yu
Re: Review Request 17476: Added a Sequence abstraction.
Posted by Jie Yu <yu...@gmail.com>.
> On Feb. 21, 2014, 1:03 a.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/include/process/sequence.hpp, line 151
> > <https://reviews.apache.org/r/17476/diff/4/?file=499673#file499673line151>
> >
> > Can you use an owned pointer for the process to save the explicit delete call in the destructor?
Hum. It's tricky because process::spawn does not accept an Owned pointer.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17476/#review35118
-----------------------------------------------------------
On Feb. 21, 2014, 12:56 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17476/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2014, 12:56 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
> One think I haven't done yet is to make it accepts '_Defer' types. (Just curious why Future.onAny can accept defer without doing something special).
>
> Also, we can get rid of the SequencerProcess if we use a mutex to protect 'last'. Not sure which one is better.
>
> The discard semantics here is a bit tricky. Basically, I wanna support discarding a single callback without affecting other callbacks. Also, when the Sequencer object is deleted, I wanna discard all the pending callbacks.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am a7d199f
> 3rdparty/libprocess/include/process/sequence.hpp PRE-CREATION
> 3rdparty/libprocess/src/tests/sequence_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17476/diff/
>
>
> Testing
> -------
>
> make check
>
> repeated 1000 times
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 17476: Added a Sequence abstraction.
Posted by Jie Yu <yu...@gmail.com>.
> On Feb. 21, 2014, 1:03 a.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/include/process/sequence.hpp, line 124
> > <https://reviews.apache.org/r/17476/diff/4/?file=499673#file499673line124>
> >
> > this could be named 'addNothing' and the next be 'addCallback' to both not require the __ and be more descriptive.
Renamed to 'completed' and 'notified' to be more descriptive.
> On Feb. 21, 2014, 1:03 a.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/include/process/sequence.hpp, line 150
> > <https://reviews.apache.org/r/17476/diff/4/?file=499673#file499673line150>
> >
> > this can move into the initializer section
> >
> > Sequence::Sequence() : process(new SequenceProcess())
> > {
> > process::spawn(process);
> > }
To be consistent with all other Process wrappers.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17476/#review35118
-----------------------------------------------------------
On Feb. 21, 2014, 9:02 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17476/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2014, 9:02 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
> One think I haven't done yet is to make it accepts '_Defer' types. (Just curious why Future.onAny can accept defer without doing something special).
>
> Also, we can get rid of the SequencerProcess if we use a mutex to protect 'last'. Not sure which one is better.
>
> The discard semantics here is a bit tricky. Basically, I wanna support discarding a single callback without affecting other callbacks. Also, when the Sequencer object is deleted, I wanna discard all the pending callbacks.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am a7d199f
> 3rdparty/libprocess/include/process/sequence.hpp PRE-CREATION
> 3rdparty/libprocess/src/tests/sequence_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17476/diff/
>
>
> Testing
> -------
>
> make check
>
> repeated 1000 times
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 17476: Added a Sequence abstraction.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17476/#review35118
-----------------------------------------------------------
3rdparty/libprocess/include/process/sequence.hpp
<https://reviews.apache.org/r/17476/#comment65527>
not for this review, but it might be good to support the use case of adding a callback that depends on the previous Future not failing.
Of course, then you could build a tree of callbacks depending on the state of the previous Futures, which may be useful in the far future.
3rdparty/libprocess/include/process/sequence.hpp
<https://reviews.apache.org/r/17476/#comment65529>
this could be named 'addNothing' and the next be 'addCallback' to both not require the __ and be more descriptive.
3rdparty/libprocess/include/process/sequence.hpp
<https://reviews.apache.org/r/17476/#comment65530>
this can move into the initializer section
Sequence::Sequence() : process(new SequenceProcess())
{
process::spawn(process);
}
3rdparty/libprocess/include/process/sequence.hpp
<https://reviews.apache.org/r/17476/#comment65531>
Can you use an owned pointer for the process to save the explicit delete call in the destructor?
- Dominic Hamon
On Feb. 20, 2014, 4:56 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17476/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2014, 4:56 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
> One think I haven't done yet is to make it accepts '_Defer' types. (Just curious why Future.onAny can accept defer without doing something special).
>
> Also, we can get rid of the SequencerProcess if we use a mutex to protect 'last'. Not sure which one is better.
>
> The discard semantics here is a bit tricky. Basically, I wanna support discarding a single callback without affecting other callbacks. Also, when the Sequencer object is deleted, I wanna discard all the pending callbacks.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am a7d199f
> 3rdparty/libprocess/include/process/sequence.hpp PRE-CREATION
> 3rdparty/libprocess/src/tests/sequence_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17476/diff/
>
>
> Testing
> -------
>
> make check
>
> repeated 1000 times
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 17476: Added a Sequence abstraction.
Posted by Jie Yu <yu...@gmail.com>.
> On Feb. 21, 2014, 9:01 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/include/process/sequence.hpp, lines 75-88
> > <https://reviews.apache.org/r/17476/diff/4/?file=499673#file499673line75>
> >
> > Awesome graph.
> >
> > Maybe N1, N2 and N3 and T1 T2 and T3 would make it even better to read?
> >
> > Also consider s/T/F/ so that it is easy to understand that N => "next/notifier" and F ==> "future" in the code.
Agreed! s/T/F/.
For the numbering part, I think the transition graph is already quite clear (from nothing -> 1 callback -> 2 callbacks), I am just too lazy to do that (you know I have to reformat:() :)
> On Feb. 21, 2014, 9:01 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/include/process/sequence.hpp, line 123
> > <https://reviews.apache.org/r/17476/diff/4/?file=499673#file499673line123>
> >
> > Why is this templated?
Good catch! It was a template before, I forgot to change it back.
> On Feb. 21, 2014, 9:01 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/include/process/sequence.hpp, line 130
> > <https://reviews.apache.org/r/17476/diff/4/?file=499673#file499673line130>
> >
> > "__add" here is a bit confusing because this is not a continuation of "_add".
Renamed to 'completed' and 'notified' to be more descriptive.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17476/#review35181
-----------------------------------------------------------
On Feb. 21, 2014, 9:02 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17476/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2014, 9:02 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
> One think I haven't done yet is to make it accepts '_Defer' types. (Just curious why Future.onAny can accept defer without doing something special).
>
> Also, we can get rid of the SequencerProcess if we use a mutex to protect 'last'. Not sure which one is better.
>
> The discard semantics here is a bit tricky. Basically, I wanna support discarding a single callback without affecting other callbacks. Also, when the Sequencer object is deleted, I wanna discard all the pending callbacks.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am a7d199f
> 3rdparty/libprocess/include/process/sequence.hpp PRE-CREATION
> 3rdparty/libprocess/src/tests/sequence_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17476/diff/
>
>
> Testing
> -------
>
> make check
>
> repeated 1000 times
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 17476: Added a Sequence abstraction.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17476/#review35181
-----------------------------------------------------------
Ship it!
LGTM. But I would like BenH to take a look.
3rdparty/libprocess/include/process/sequence.hpp
<https://reviews.apache.org/r/17476/#comment65607>
s/this/these/ ?
s/register/registering/
3rdparty/libprocess/include/process/sequence.hpp
<https://reviews.apache.org/r/17476/#comment65613>
Awesome graph.
Maybe N1, N2 and N3 and T1 T2 and T3 would make it even better to read?
Also consider s/T/F/ so that it is easy to understand that N => "next/notifier" and F ==> "future" in the code.
3rdparty/libprocess/include/process/sequence.hpp
<https://reviews.apache.org/r/17476/#comment65616>
s/arrow// ?
3rdparty/libprocess/include/process/sequence.hpp
<https://reviews.apache.org/r/17476/#comment65617>
s/arrow// ?
s/'N'/ previous 'N'/
3rdparty/libprocess/include/process/sequence.hpp
<https://reviews.apache.org/r/17476/#comment65636>
Why is this templated?
3rdparty/libprocess/include/process/sequence.hpp
<https://reviews.apache.org/r/17476/#comment65628>
We typically prefix "_" for methods that are continuations.
Though in this particular case "_add" is not a continuation of "add"
3rdparty/libprocess/include/process/sequence.hpp
<https://reviews.apache.org/r/17476/#comment65629>
"__add" here is a bit confusing because this is not a continuation of "_add".
3rdparty/libprocess/src/tests/sequence_tests.cpp
<https://reviews.apache.org/r/17476/#comment65641>
Add a comment on what this test is testing?
3rdparty/libprocess/src/tests/sequence_tests.cpp
<https://reviews.apache.org/r/17476/#comment65642>
Ditto. Comment.
3rdparty/libprocess/src/tests/sequence_tests.cpp
<https://reviews.apache.org/r/17476/#comment65643>
Ditto. Comment.
- Vinod Kone
On Feb. 21, 2014, 5:03 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17476/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2014, 5:03 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
> One think I haven't done yet is to make it accepts '_Defer' types. (Just curious why Future.onAny can accept defer without doing something special).
>
> Also, we can get rid of the SequencerProcess if we use a mutex to protect 'last'. Not sure which one is better.
>
> The discard semantics here is a bit tricky. Basically, I wanna support discarding a single callback without affecting other callbacks. Also, when the Sequencer object is deleted, I wanna discard all the pending callbacks.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am a7d199f
> 3rdparty/libprocess/include/process/sequence.hpp PRE-CREATION
> 3rdparty/libprocess/src/tests/sequence_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17476/diff/
>
>
> Testing
> -------
>
> make check
>
> repeated 1000 times
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 17476: Added a Sequence abstraction.
Posted by Jie Yu <yu...@gmail.com>.
> On Feb. 27, 2014, 9:04 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/sequence.hpp, lines 33-36
> > <https://reviews.apache.org/r/17476/diff/5/?file=505027#file505027line33>
> >
> > Can you mention explicitly that discarding the result only attempts to discard the provided callback, and the sequence will otherwise _continue_?
> >
> > I'm thinking some might be confused about whether discard() would continue through the remaining callbacks. What I mean is:
> >
> > C1 C2 C3 C4
> > ^
> > C2 discarded
> >
> > C3 and C4 will still execute after C2 is discarded.
> >
> > Do you think this is obvious from the abstraction itself? Are the callbacks expected to be independent?
Added comments.
> On Feb. 27, 2014, 9:04 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/sequence.hpp, lines 57-110
> > <https://reviews.apache.org/r/17476/diff/5/?file=505027#file505027line57>
> >
> > I wonder if we can make this a bit easier to grasp, having to understand what 'next', 'previous', 'last', 'promise' and 'future' meant here was a bit tricky (especially because we swap 'last' at the beginning; so throughout the function 'next' and 'last' are effectively the same thing!). A few suggestions that might help:
> >
> > 1. Swap 'next' with 'last' at the end rather than the beginning. This means we can kill 'previous' as well!
> >
> > 2. 'promise' and 'future' are pretty generic names, can they be more meaningful? Like 'notifier'? On this note, what about 'result' for the one we're returning?
> >
> > Something like:
> > ---------------------------------
> > Before
> > ---------------------------------
> > Owned<Promise<Nothing> > next(new Promise<Nothing>());
> > Future<Nothing> previous = last;
> > last = next->future();
> >
> > Owned<Promise<T> > promise(new Promise<T>());
> > Future<T> future = promise->future();
> >
> > future.onAny(lambda::bind(&completed, next));
> >
> > previous.onAny(lambda::bind(¬ified<T>, promise, callback));
> >
> > last.onDiscard(lambda::bind(&internal::discard<T>, WeakFuture<T>(future)));
> >
> > last.onDiscard(lambda::bind(
> > &internal::discard<Nothing>,
> > WeakFuture<Nothing>(previous)));
> >
> > return future;
> >
> >
> > ----------------------------------
> > After
> > ----------------------------------
> > Owned<Promise<Nothing> > notifier(new Promise<Nothing>());
> > Owned<Promise<T> > result(new Promise<T>());
> >
> > result->future().onAny(lambda::bind(&completed, notifier));
> >
> > last.onAny(lambda::bind(¬ified<T>, result, callback));
> >
> > notifier->future().onDiscard(lambda::bind(&internal::discard<T>, WeakFuture<T>(result->future())));
> >
> > notifier->future().onDiscard(lambda::bind(
> > &internal::discard<Nothing>,
> > WeakFuture<Nothing>(last)));
> >
> > last = next->future();
> >
> > return future;
Liked you suggestion! Moved updating 'last' to the end.
However, I don't like the name 'result'. In our code base (e.g. then, collect, etc.), we usually use the name 'promise/future' to represent the promise/future that will be returned to the user, I'd like to keep that:)
> On Feb. 27, 2014, 9:04 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/sequence_tests.cpp, lines 66-68
> > <https://reviews.apache.org/r/17476/diff/5/?file=505028#file505028line66>
> >
> > Maybe add a note as to why you're doing this?
Added a comment.
> On Feb. 27, 2014, 9:04 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/sequence_tests.cpp, line 70
> > <https://reviews.apache.org/r/17476/diff/5/?file=505028#file505028line70>
> >
> > Can't wait for EXPECT_PENDING! :)
Will do it soon in a different review!
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17476/#review35697
-----------------------------------------------------------
On Feb. 26, 2014, 10:51 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17476/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2014, 10:51 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
> One think I haven't done yet is to make it accepts '_Defer' types. (Just curious why Future.onAny can accept defer without doing something special).
>
> Also, we can get rid of the SequencerProcess if we use a mutex to protect 'last'. Not sure which one is better.
>
> The discard semantics here is a bit tricky. Basically, I wanna support discarding a single callback without affecting other callbacks. Also, when the Sequencer object is deleted, I wanna discard all the pending callbacks.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am a7d199f
> 3rdparty/libprocess/include/process/sequence.hpp PRE-CREATION
> 3rdparty/libprocess/src/tests/sequence_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17476/diff/
>
>
> Testing
> -------
>
> make check
>
> repeated 1000 times
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 17476: Added a Sequence abstraction.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17476/#review35697
-----------------------------------------------------------
Ship it!
Looks great! Mostly I'm just wondering if we can make Sequence::add a bit easier to understand.
3rdparty/libprocess/include/process/sequence.hpp
<https://reviews.apache.org/r/17476/#comment66402>
Can you mention explicitly that discarding the result only attempts to discard the provided callback, and the sequence will otherwise _continue_?
I'm thinking some might be confused about whether discard() would continue through the remaining callbacks. What I mean is:
C1 C2 C3 C4
^
C2 discarded
C3 and C4 will still execute after C2 is discarded.
Do you think this is obvious from the abstraction itself? Are the callbacks expected to be independent?
3rdparty/libprocess/include/process/sequence.hpp
<https://reviews.apache.org/r/17476/#comment66403>
"he/she" or just use "they" :)
3rdparty/libprocess/include/process/sequence.hpp
<https://reviews.apache.org/r/17476/#comment66412>
I wonder if we can make this a bit easier to grasp, having to understand what 'next', 'previous', 'last', 'promise' and 'future' meant here was a bit tricky (especially because we swap 'last' at the beginning; so throughout the function 'next' and 'last' are effectively the same thing!). A few suggestions that might help:
1. Swap 'next' with 'last' at the end rather than the beginning. This means we can kill 'previous' as well!
2. 'promise' and 'future' are pretty generic names, can they be more meaningful? Like 'notifier'? On this note, what about 'result' for the one we're returning?
Something like:
---------------------------------
Before
---------------------------------
Owned<Promise<Nothing> > next(new Promise<Nothing>());
Future<Nothing> previous = last;
last = next->future();
Owned<Promise<T> > promise(new Promise<T>());
Future<T> future = promise->future();
future.onAny(lambda::bind(&completed, next));
previous.onAny(lambda::bind(¬ified<T>, promise, callback));
last.onDiscard(lambda::bind(&internal::discard<T>, WeakFuture<T>(future)));
last.onDiscard(lambda::bind(
&internal::discard<Nothing>,
WeakFuture<Nothing>(previous)));
return future;
----------------------------------
After
----------------------------------
Owned<Promise<Nothing> > notifier(new Promise<Nothing>());
Owned<Promise<T> > result(new Promise<T>());
result->future().onAny(lambda::bind(&completed, notifier));
last.onAny(lambda::bind(¬ified<T>, result, callback));
notifier->future().onDiscard(lambda::bind(&internal::discard<T>, WeakFuture<T>(result->future())));
notifier->future().onDiscard(lambda::bind(
&internal::discard<Nothing>,
WeakFuture<Nothing>(last)));
last = next->future();
return future;
3rdparty/libprocess/include/process/sequence.hpp
<https://reviews.apache.org/r/17476/#comment66407>
s/;;/;/
3rdparty/libprocess/include/process/sequence.hpp
<https://reviews.apache.org/r/17476/#comment66408>
s/done/completed/
3rdparty/libprocess/src/tests/sequence_tests.cpp
<https://reviews.apache.org/r/17476/#comment66414>
Maybe add a note as to why you're doing this?
3rdparty/libprocess/src/tests/sequence_tests.cpp
<https://reviews.apache.org/r/17476/#comment66415>
Can't wait for EXPECT_PENDING! :)
3rdparty/libprocess/src/tests/sequence_tests.cpp
<https://reviews.apache.org/r/17476/#comment66419>
Ditto.
3rdparty/libprocess/src/tests/sequence_tests.cpp
<https://reviews.apache.org/r/17476/#comment66418>
Ditto.
3rdparty/libprocess/src/tests/sequence_tests.cpp
<https://reviews.apache.org/r/17476/#comment66416>
Should there be an expectation on f0 as well?
3rdparty/libprocess/src/tests/sequence_tests.cpp
<https://reviews.apache.org/r/17476/#comment66417>
Ditto.
- Ben Mahler
On Feb. 26, 2014, 10:51 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17476/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2014, 10:51 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
> One think I haven't done yet is to make it accepts '_Defer' types. (Just curious why Future.onAny can accept defer without doing something special).
>
> Also, we can get rid of the SequencerProcess if we use a mutex to protect 'last'. Not sure which one is better.
>
> The discard semantics here is a bit tricky. Basically, I wanna support discarding a single callback without affecting other callbacks. Also, when the Sequencer object is deleted, I wanna discard all the pending callbacks.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am a7d199f
> 3rdparty/libprocess/include/process/sequence.hpp PRE-CREATION
> 3rdparty/libprocess/src/tests/sequence_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17476/diff/
>
>
> Testing
> -------
>
> make check
>
> repeated 1000 times
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 17476: Added a Sequence abstraction.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17476/
-----------------------------------------------------------
(Updated Feb. 28, 2014, 10:47 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
BenM's comments.
Repository: mesos-git
Description
-------
See summary.
One think I haven't done yet is to make it accepts '_Defer' types. (Just curious why Future.onAny can accept defer without doing something special).
Also, we can get rid of the SequencerProcess if we use a mutex to protect 'last'. Not sure which one is better.
The discard semantics here is a bit tricky. Basically, I wanna support discarding a single callback without affecting other callbacks. Also, when the Sequencer object is deleted, I wanna discard all the pending callbacks.
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am a7d199f
3rdparty/libprocess/include/process/sequence.hpp PRE-CREATION
3rdparty/libprocess/src/tests/sequence_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/17476/diff/
Testing
-------
make check
repeated 1000 times
Thanks,
Jie Yu
Re: Review Request 17476: Added a Sequence abstraction.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17476/
-----------------------------------------------------------
(Updated Feb. 26, 2014, 10:51 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Dominic and Vinod's comments. Rebased.
Repository: mesos-git
Description
-------
See summary.
One think I haven't done yet is to make it accepts '_Defer' types. (Just curious why Future.onAny can accept defer without doing something special).
Also, we can get rid of the SequencerProcess if we use a mutex to protect 'last'. Not sure which one is better.
The discard semantics here is a bit tricky. Basically, I wanna support discarding a single callback without affecting other callbacks. Also, when the Sequencer object is deleted, I wanna discard all the pending callbacks.
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am a7d199f
3rdparty/libprocess/include/process/sequence.hpp PRE-CREATION
3rdparty/libprocess/src/tests/sequence_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/17476/diff/
Testing
-------
make check
repeated 1000 times
Thanks,
Jie Yu
Re: Review Request 17476: Added a Sequence abstraction.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17476/
-----------------------------------------------------------
(Updated Feb. 21, 2014, 9:02 p.m.)
Review request for mesos and Benjamin Hindman.
Repository: mesos-git
Description
-------
See summary.
One think I haven't done yet is to make it accepts '_Defer' types. (Just curious why Future.onAny can accept defer without doing something special).
Also, we can get rid of the SequencerProcess if we use a mutex to protect 'last'. Not sure which one is better.
The discard semantics here is a bit tricky. Basically, I wanna support discarding a single callback without affecting other callbacks. Also, when the Sequencer object is deleted, I wanna discard all the pending callbacks.
Diffs
-----
3rdparty/libprocess/Makefile.am a7d199f
3rdparty/libprocess/include/process/sequence.hpp PRE-CREATION
3rdparty/libprocess/src/tests/sequence_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/17476/diff/
Testing
-------
make check
repeated 1000 times
Thanks,
Jie Yu
Re: Review Request 17476: Added a Sequence abstraction.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17476/
-----------------------------------------------------------
(Updated Feb. 21, 2014, 5:03 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
Added BenM to the reviewers.
Repository: mesos-git
Description
-------
See summary.
One think I haven't done yet is to make it accepts '_Defer' types. (Just curious why Future.onAny can accept defer without doing something special).
Also, we can get rid of the SequencerProcess if we use a mutex to protect 'last'. Not sure which one is better.
The discard semantics here is a bit tricky. Basically, I wanna support discarding a single callback without affecting other callbacks. Also, when the Sequencer object is deleted, I wanna discard all the pending callbacks.
Diffs
-----
3rdparty/libprocess/Makefile.am a7d199f
3rdparty/libprocess/include/process/sequence.hpp PRE-CREATION
3rdparty/libprocess/src/tests/sequence_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/17476/diff/
Testing
-------
make check
repeated 1000 times
Thanks,
Jie Yu
Re: Review Request 17476: Added a Sequence abstraction.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17476/
-----------------------------------------------------------
(Updated Feb. 21, 2014, 12:56 a.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Simplified it with the new discard semantics. Rebased.
Repository: mesos-git
Description
-------
See summary.
One think I haven't done yet is to make it accepts '_Defer' types. (Just curious why Future.onAny can accept defer without doing something special).
Also, we can get rid of the SequencerProcess if we use a mutex to protect 'last'. Not sure which one is better.
The discard semantics here is a bit tricky. Basically, I wanna support discarding a single callback without affecting other callbacks. Also, when the Sequencer object is deleted, I wanna discard all the pending callbacks.
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am a7d199f
3rdparty/libprocess/include/process/sequence.hpp PRE-CREATION
3rdparty/libprocess/src/tests/sequence_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/17476/diff/
Testing
-------
make check
repeated 1000 times
Thanks,
Jie Yu
Re: Review Request 17476: Added a Sequence abstraction.
Posted by Jie Yu <yu...@gmail.com>.
> On Jan. 30, 2014, 7:41 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/sequence_tests.cpp, lines 61-62
> > <https://reviews.apache.org/r/17476/diff/3/?file=456680#file456680line61>
> >
> > I'm a little confused with the expected use case, why would someone not just use .onAny or .then?
> >
> > In this example they have the foresight to now that they want bar called after foo, which means a .then or .onAny would be equivalent?
> >
> > Is Sequence meant to be kept around, and have things added to it in multiple contexts?
Ben, see the Sequence.Random test. Sometimes, users may want atomicity but do not care about the order. Think about a Sequence object as a mutex in the thread-based world.
We need that in the new LogStorage for State:
https://reviews.apache.org/r/17423/
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17476/#review33239
-----------------------------------------------------------
On Jan. 30, 2014, 6:06 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17476/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2014, 6:06 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
> One think I haven't done yet is to make it accepts '_Defer' types. (Just curious why Future.onAny can accept defer without doing something special).
>
> Also, we can get rid of the SequencerProcess if we use a mutex to protect 'last'. Not sure which one is better.
>
> The discard semantics here is a bit tricky. Basically, I wanna support discarding a single callback without affecting other callbacks. Also, when the Sequencer object is deleted, I wanna discard all the pending callbacks.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am bbd17cc
> 3rdparty/libprocess/include/process/sequence.hpp PRE-CREATION
> 3rdparty/libprocess/src/tests/sequence_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17476/diff/
>
>
> Testing
> -------
>
> make check
>
> repeated 1000 times
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 17476: Added a Sequence abstraction.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17476/#review33239
-----------------------------------------------------------
3rdparty/libprocess/src/tests/sequence_tests.cpp
<https://reviews.apache.org/r/17476/#comment62621>
I'm a little confused with the expected use case, why would someone not just use .onAny or .then?
In this example they have the foresight to now that they want bar called after foo, which means a .then or .onAny would be equivalent?
Is Sequence meant to be kept around, and have things added to it in multiple contexts?
- Ben Mahler
On Jan. 30, 2014, 6:06 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17476/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2014, 6:06 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
> One think I haven't done yet is to make it accepts '_Defer' types. (Just curious why Future.onAny can accept defer without doing something special).
>
> Also, we can get rid of the SequencerProcess if we use a mutex to protect 'last'. Not sure which one is better.
>
> The discard semantics here is a bit tricky. Basically, I wanna support discarding a single callback without affecting other callbacks. Also, when the Sequencer object is deleted, I wanna discard all the pending callbacks.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am bbd17cc
> 3rdparty/libprocess/include/process/sequence.hpp PRE-CREATION
> 3rdparty/libprocess/src/tests/sequence_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17476/diff/
>
>
> Testing
> -------
>
> make check
>
> repeated 1000 times
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 17476: Added a Sequence abstraction.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17476/
-----------------------------------------------------------
(Updated Jan. 30, 2014, 6:06 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Rebased.
Repository: mesos-git
Description
-------
See summary.
One think I haven't done yet is to make it accepts '_Defer' types. (Just curious why Future.onAny can accept defer without doing something special).
Also, we can get rid of the SequencerProcess if we use a mutex to protect 'last'. Not sure which one is better.
The discard semantics here is a bit tricky. Basically, I wanna support discarding a single callback without affecting other callbacks. Also, when the Sequencer object is deleted, I wanna discard all the pending callbacks.
Diffs (updated)
-----
3rdparty/libprocess/Makefile.am bbd17cc
3rdparty/libprocess/include/process/sequence.hpp PRE-CREATION
3rdparty/libprocess/src/tests/sequence_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/17476/diff/
Testing
-------
make check
repeated 1000 times
Thanks,
Jie Yu