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