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