----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25945/#review54571 -----------------------------------------------------------
This seems like a good approach, I'm curious if there's any room for cleanup now that we've split the types of Dispatch events. Can you add benh to the reviewers for this patch? It would be great if you could split this patch so that the cleanups you did (shared_ptr -> Owned, assertions -> CHECK, etc) are in a separate patch from the change in semantics, thanks! 3rdparty/libprocess/include/process/dispatch.hpp <https://reviews.apache.org/r/25945/#comment94765> Could the left hand side store a DispatchEvent*? 3rdparty/libprocess/include/process/event.hpp <https://reviews.apache.org/r/25945/#comment94755> Do you need this forward declaration? Or, can we remove the future.hpp include? 3rdparty/libprocess/include/process/event.hpp <https://reviews.apache.org/r/25945/#comment94766> Do we want to add an explicit virtual destructor here per the other Events above? Ditto in PromiseDispatchEvent. 3rdparty/libprocess/include/process/event.hpp <https://reviews.apache.org/r/25945/#comment94763> Could you add a comment here that this is a DispatchEvent with a result? s/R/T/ 3rdparty/libprocess/include/process/event.hpp <https://reviews.apache.org/r/25945/#comment94764> Can you be more explicit with .get()? I'm looking at shared.hpp and seeing that we avoided the implicit bool operator. src/tests/master_tests.cpp <https://reviews.apache.org/r/25945/#comment94761> Instead of writing an integration test within Mesos, what about writing some simple libprocess tests that demonstrate the added semantics of dispatch: (1) Process terminated: discarded. (2) Non-existent UPID: discarded. - Ben Mahler On Sept. 23, 2014, 5:24 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25945/ > ----------------------------------------------------------- > > (Updated Sept. 23, 2014, 5:24 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-1456 and MESOS-1751 > https://issues.apache.org/jira/browse/MESOS-1456 > https://issues.apache.org/jira/browse/MESOS-1751 > > > Repository: mesos-git > > > Description > ------- > > Create a PromiseDispatchEvent to allow discarding of promises if the > DispatchEvent can't be enqueued. > > Also converted some shared_ptr to Owned to correctly track ownership. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/c++11/dispatch.hpp > 76da2828cf36b6143d627dac66f3e0cc4416bae4 > 3rdparty/libprocess/include/process/defer.hpp > ebe6f2db47cab2a3306288d8ebabb720e7cd8976 > 3rdparty/libprocess/include/process/delay.hpp > 487f652c9e9b7c8c3aa8b4edc9e59789cffd8da9 > 3rdparty/libprocess/include/process/dispatch.hpp > 88570f7c078faa7d79b2c187aa6a15e4e939878c > 3rdparty/libprocess/include/process/event.hpp > bf689d7270df2c8f1f5c9165d2bbcfdca06e15a8 > 3rdparty/libprocess/src/process.cpp > 8adc8093ab08a51c437b18dc37a7e0ae3f56870b > src/tests/master_tests.cpp 8e4ec1d85c4530b5421387de55036f7d40ee3180 > > Diff: https://reviews.apache.org/r/25945/diff/ > > > Testing > ------- > > Added test that fails without the patch. > > Ran all tests (make check) with g++-4.4 and clang-3.5. > > > Thanks, > > Dominic Hamon > >
