----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16290/#review30628 -----------------------------------------------------------
Looking much better, thanks Yan! src/master/master.cpp <https://reviews.apache.org/r/16290/#comment58673> Just a thought for later: was there a need for initialize or could the contend() call have taken the UPID as an argument? src/master/master.cpp <https://reviews.apache.org/r/16290/#comment58678> No need for an else block now that we always EXIT. src/sched/sched.cpp <https://reviews.apache.org/r/16290/#comment58683> I see you updated this code in r/16291 but I think it belongs in this change since you've made the same update to the Master's detection code. src/slave/slave.cpp <https://reviews.apache.org/r/16290/#comment58689> Ditto on updating failure handling in this change instead of r/16291. src/tests/master_contender_detector_tests.cpp <https://reviews.apache.org/r/16290/#comment58691> Can we keep this test but ensure that it does not fail? src/tests/zookeeper_tests.cpp <https://reviews.apache.org/r/16290/#comment58693> Ditto, is it possible to keep this and ensure we handle this without failing? src/zookeeper/contender.cpp <https://reviews.apache.org/r/16290/#comment58708> Looks like this is a failure to contend rather than a consistency issue, so it seems Failure is more appropriate here. src/zookeeper/contender.cpp <https://reviews.apache.org/r/16290/#comment58699> I'm not sure this should be a CHECK failure since it can be handled gracefully. Can this return a failure instead? src/zookeeper/contender.cpp <https://reviews.apache.org/r/16290/#comment58700> Is this also a Failure to withdraw? src/zookeeper/contender.cpp <https://reviews.apache.org/r/16290/#comment58717> CHECK_SOME src/zookeeper/group.hpp <https://reviews.apache.org/r/16290/#comment58719> remove const& src/zookeeper/group.cpp <https://reviews.apache.org/r/16290/#comment58720> You could rename this to message, and pass 'message' into the second argument for the calls to fail(), might be a bit cleaner. - Ben Mahler On Dec. 18, 2013, 6 a.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16290/ > ----------------------------------------------------------- > > (Updated Dec. 18, 2013, 6 a.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone. > > > Bugs: MESOS-883 > https://issues.apache.org/jira/browse/MESOS-883 > > > Repository: mesos-git > > > Description > ------- > > With this patch the group only returns failed futures when the error is > non-retryable and client (master, slave, sched) should always terminate. > > > Diffs > ----- > > src/master/master.hpp 6c168a2cdbd8343516cb47adceaff70c3d46690b > src/master/master.cpp dd6111946289b82008a66e46df8ef5e538de70ea > src/sched/sched.cpp c46535643ce2ea456bd939571fe78358c1d3871b > src/slave/slave.cpp 6e6107e3551f29566ff233dad47dac8c29b4fab5 > src/tests/master_contender_detector_tests.cpp > 76464eab479461e6e3cb8b5afe85860e60428cf5 > src/tests/zookeeper_tests.cpp a0660cbfb1b7073654b05b82bffd69fbb04d0165 > src/zookeeper/contender.hpp d0b386eeec485f7c8e6b5e60936a84d2f0bba92d > src/zookeeper/contender.cpp bb7e25555b2635398e9d56a808993bcd0b0e47f6 > src/zookeeper/group.hpp 088363a80108472ffeb14121fcb4da02d6e9acf4 > src/zookeeper/group.cpp f3ba86fec3584a83fd55d6388656ebde5249824a > > Diff: https://reviews.apache.org/r/16290/diff/ > > > Testing > ------- > > make check. > > > Thanks, > > Jiang Yan Xu > >
