> On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote: > > src/master/detector.hpp, lines 66-67 > > <https://reviews.apache.org/r/16291/diff/1/?file=398168#file398168line66> > > > > This feels odd to me (see my comment below) since we haven't > > encountered a failure in this case.
This is now obsolete because we are indeed returning failed futures for "errors". > On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote: > > src/master/detector.hpp, line 90 > > <https://reviews.apache.org/r/16291/diff/1/?file=398168#file398168line90> > > > > What does it mean to be "detected by default"? Removed "by default". Here "can be *detected*" just means "we are really telling the detector what should be detected". > On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote: > > src/master/detector.hpp, line 91 > > <https://reviews.apache.org/r/16291/diff/1/?file=398168#file398168line91> > > > > I'm not sure why you need to call this out. Given the argument is an > > Option, None can be passed. Okay > On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote: > > src/master/detector.hpp, lines 96-97 > > <https://reviews.apache.org/r/16291/diff/1/?file=398168#file398168line96> > > > > This is no longer true? Killed the paragraph. > On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote: > > src/master/detector.cpp, line 150 > > <https://reviews.apache.org/r/16291/diff/1/?file=398169#file398169line150> > > > > We're using a failure here for what seems like a discard. Any reason? > On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote: > > src/master/detector.cpp, lines 312-314 > > <https://reviews.apache.org/r/16291/diff/1/?file=398169#file398169line312> > > > > It's a bit concerning that we do the group->data operation and the > > detector.detect operation at the same time. Have you thought about what > > might happen if, say, there's a race and the detected() callback happens > > before fetched()? I think it's fine. If the detect() returns first, the result of fetched() may not be the most up-to-date but it will be refreshed soon. It's not different from the alternative: calling detect() in fetched() - leader could have changed on the server. Moreover, we are not saving the Group::Membership returned in detected (at least for now) which is needed for re-detecting. > On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote: > > src/sched/sched.cpp, line 421 > > <https://reviews.apache.org/r/16291/diff/1/?file=398172#file398172line421> > > > > All the message handlers look at !master.isSome(), can you clean those > > up as well? I think this file is all cleaned up. There are some master.isSome() without "!" > On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote: > > src/slave/slave.cpp, line 432 > > <https://reviews.apache.org/r/16291/diff/1/?file=398174#file398174line432> > > > > We may want to make equality operators for Option<T> vs T so that these > > comparisons can be cleaned up to be just: > > > > if (from && master != from) { Done. > On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote: > > src/zookeeper/detector.hpp, line 30 > > <https://reviews.apache.org/r/16291/diff/1/?file=398177#file398177line30> > > > > s/some/a/ Done. > On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote: > > src/zookeeper/detector.hpp, lines 31-32 > > <https://reviews.apache.org/r/16291/diff/1/?file=398177#file398177line31> > > > > We probably should not refer to a 'user' here. How about: > > > > // Note that the detector transparently tries to recover from retryable > > errors. Done. > On Dec. 17, 2013, 1:44 a.m., Ben Mahler wrote: > > src/master/detector.cpp, lines 325-333 > > <https://reviews.apache.org/r/16291/diff/1/?file=398169#file398169line325> > > > > Shouldn't this be setting the error field? Removed error field as it's unnecessary - we can let future detector::detect() proceed and group will return failures directly. - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16291/#review30505 ----------------------------------------------------------- On Dec. 16, 2013, 10:12 a.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16291/ > ----------------------------------------------------------- > > (Updated Dec. 16, 2013, 10:12 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 > ------- > > Now that local session timeouts are transparent to the clients of the master > detector, they are not supposed to retry anymore. Thus passing "Result<UPID> > previous" back into MasterDetector::detect() is no longer correct. > > > Diffs > ----- > > src/cli/resolve.cpp dddadfc2bff41752372e0e817beb0493cd0cd127 > src/master/detector.hpp 6e7a4c42c8a1dcfe95b2244213013d3f0aff311a > src/master/detector.cpp 2f73f669f0b3a8dccbce9fdfa941593314302171 > src/master/master.hpp 6c168a2cdbd8343516cb47adceaff70c3d46690b > src/master/master.cpp dd6111946289b82008a66e46df8ef5e538de70ea > src/sched/sched.cpp c46535643ce2ea456bd939571fe78358c1d3871b > src/slave/slave.hpp 71fa4f0d0ccde8cdc023b18c6a4d7eb7478fd0cf > src/slave/slave.cpp 6e6107e3551f29566ff233dad47dac8c29b4fab5 > src/tests/master_contender_detector_tests.cpp > 76464eab479461e6e3cb8b5afe85860e60428cf5 > src/tests/zookeeper_tests.cpp a0660cbfb1b7073654b05b82bffd69fbb04d0165 > src/zookeeper/detector.hpp de4acab422e2fa4b7420f44b4e8ebc941f91f200 > src/zookeeper/detector.cpp 1de3663ecaed6d8c362c20a5a46d2f4d73f8fbd0 > > Diff: https://reviews.apache.org/r/16291/diff/ > > > Testing > ------- > > make check. > > > Thanks, > > Jiang Yan Xu > >
