----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60008/#review178888 -----------------------------------------------------------
3rdparty/libprocess/include/process/dispatch.hpp Lines 68 (patched) <https://reviews.apache.org/r/60008/#comment253224> Why don't you use `Option<>`? 3rdparty/libprocess/include/process/dispatch.hpp Lines 149 (patched) <https://reviews.apache.org/r/60008/#comment253223> `canonicalizePointer`? 3rdparty/libprocess/include/process/event.hpp Lines 178-179 (patched) <https://reviews.apache.org/r/60008/#comment253225> Please a blank line here, i.e., ``` // Any society that would give up a little liberty to gain // a little security will deserve neither and lose both. // // TODO(abudnik): Add a source link for the quote above. ``` 3rdparty/libprocess/src/tests/process_tests.cpp Line 189 (original), 190-192 (patched) <https://reviews.apache.org/r/60008/#comment253221> We strive to add a descriptive comment for each test. Even though it is not an easy to go and add comments to existing tests, we do want to have a proper description for every newly added test. 3rdparty/libprocess/src/tests/process_tests.cpp Lines 204-205 (patched) <https://reviews.apache.org/r/60008/#comment253220> Why do you need this statement? 3rdparty/libprocess/src/tests/process_tests.cpp Lines 208-209 (patched) <https://reviews.apache.org/r/60008/#comment253222> Additionally to (or actually instead of) `EXPECT` here, `AWAIT_READY(f);` would carry your intent better, I think. I would actually get rid of call expectations all together and simply focus on future states. - Alexander Rukletsov On June 16, 2017, 5:34 p.m., Andrei Budnik wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60008/ > ----------------------------------------------------------- > > (Updated June 16, 2017, 5:34 p.m.) > > > Review request for mesos, Alexander Rukletsov, Michael Park, and Neil Conway. > > > Bugs: MESOS-5886 > https://issues.apache.org/jira/browse/MESOS-5886 > > > Repository: mesos > > > Description > ------- > > FUTURE_DISPATCH uses DispatchMatcher to figure out whether a processed > DispatchEvent is the same the user is waiting for. Currently, we > compare std::type_info of function pointers, which is not enough: > different class methods with same signatures will be matched (see > MESOS-5886 for an example). > This patch adds value of pointer-to-member function in addition to > std::type_info in DispatchEvent to uniquely identify class methods. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/dispatch.hpp > 3a0793888dc0df5e3ec31b06f47cd920c71e0db9 > 3rdparty/libprocess/include/process/event.hpp > 8afe6266eb0dc5a17af35d79efb6bfdf9e6a0ee9 > 3rdparty/libprocess/include/process/gmock.hpp > e9af943b39436f365fe687301febb5c7fbefffc4 > 3rdparty/libprocess/src/process.cpp > 4ff7448d171f39dbb8cbb81dd9bed136ad43d62d > 3rdparty/libprocess/src/tests/process_tests.cpp > 38d787a083a5eb31e922d283f4b4bed2bd62eb0a > > > Diff: https://reviews.apache.org/r/60008/diff/1/ > > > Testing > ------- > > 1. make check (mac os x 10.12, fedora 25) > 2. internal CI > > NOTE: Test GroupTest.ConnectTimer is broken, because it uses FUTURE_DISPATCH > on a wrong method (GroupProcess::expired), > which has the same signature as a private method (GroupProcess::timedout). > The later method is actually called, > thus causing the error after applying this patch. > > > Thanks, > > Andrei Budnik > >
