----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16291/#review30505 -----------------------------------------------------------
I've commented on some bugs and cleanup here, as well as some cleanup leftover from the original changes. With the latter, you may want to consider addressing these in an earlier change. src/cli/resolve.cpp <https://reviews.apache.org/r/16291/#comment58372> Can you check directly against discarded? We can check this above the failure check: } else { CHECK(!pid.isDiscarded()); if (pid.isFailed()) { ... } } src/master/detector.hpp <https://reviews.apache.org/r/16291/#comment58388> Returns a ...? src/master/detector.hpp <https://reviews.apache.org/r/16291/#comment58387> s/some/a/ src/master/detector.hpp <https://reviews.apache.org/r/16291/#comment58389> See my other comment about referring to a 'user'. src/master/detector.hpp <https://reviews.apache.org/r/16291/#comment58399> This feels odd to me (see my comment below) since we haven't encountered a failure in this case. src/master/detector.hpp <https://reviews.apache.org/r/16291/#comment58402> What does it mean to be "detected by default"? src/master/detector.hpp <https://reviews.apache.org/r/16291/#comment58390> I'm not sure why you need to call this out. Given the argument is an Option, None can be passed. src/master/detector.hpp <https://reviews.apache.org/r/16291/#comment58403> This is no longer true? src/master/detector.hpp <https://reviews.apache.org/r/16291/#comment58404> In general I don't think it's necessary to mention things like "A constructor overload" here since it is self-evident. It seems sufficient to say: // Used for testing purposes. src/master/detector.cpp <https://reviews.apache.org/r/16291/#comment58405> Option<UPID> leader; // The appointed master. src/master/detector.cpp <https://reviews.apache.org/r/16291/#comment58407> Feel free to just kill this comment. src/master/detector.cpp <https://reviews.apache.org/r/16291/#comment58406> This comment seems out of place. src/master/detector.cpp <https://reviews.apache.org/r/16291/#comment58391> Ditto my other comment below about 'operable'. src/master/detector.cpp <https://reviews.apache.org/r/16291/#comment58392> Ditto about Option<Error>. src/master/detector.cpp <https://reviews.apache.org/r/16291/#comment58393> We're using a failure here for what seems like a discard. Any reason? src/master/detector.cpp <https://reviews.apache.org/r/16291/#comment58408> You're missing an if in this sentence? This comment and the one in ZK should just be the same, no? I probably would kill this comment since the code is now much simpler. src/master/detector.cpp <https://reviews.apache.org/r/16291/#comment58409> Ditto about discarded. src/master/detector.cpp <https://reviews.apache.org/r/16291/#comment58410> Ditto about killing this comment now that the code is obvious. src/master/detector.cpp <https://reviews.apache.org/r/16291/#comment58411> Ditto about checking for discarded explicitly and not attributing the discard to anyone specifically. src/master/detector.cpp <https://reviews.apache.org/r/16291/#comment58412> 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()? src/master/detector.cpp <https://reviews.apache.org/r/16291/#comment58414> Shouldn't this be setting the error field? src/master/master.cpp <https://reviews.apache.org/r/16291/#comment58420> Ditto about explicitly checking and not attributing the discard to a particular component. src/sched/sched.cpp <https://reviews.apache.org/r/16291/#comment58417> Ditto about checking for discard directly. src/sched/sched.cpp <https://reviews.apache.org/r/16291/#comment58418> else now? src/sched/sched.cpp <https://reviews.apache.org/r/16291/#comment58419> All the message handlers look at !master.isSome(), can you clean those up as well? src/slave/slave.cpp <https://reviews.apache.org/r/16291/#comment58421> 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) { src/slave/slave.cpp <https://reviews.apache.org/r/16291/#comment58422> Ditto discard/attribution. src/slave/slave.cpp <https://reviews.apache.org/r/16291/#comment58424> Just else now? src/tests/master_contender_detector_tests.cpp <https://reviews.apache.org/r/16291/#comment58384> No need to store joined in a variable. src/tests/master_contender_detector_tests.cpp <https://reviews.apache.org/r/16291/#comment58385> No need to store result in a variable. src/zookeeper/detector.hpp <https://reviews.apache.org/r/16291/#comment58374> s/some/a/ src/zookeeper/detector.hpp <https://reviews.apache.org/r/16291/#comment58375> We probably should not refer to a 'user' here. How about: // Note that the detector transparently tries to recover from retryable errors. src/zookeeper/detector.cpp <https://reviews.apache.org/r/16291/#comment58376> What does no longer operable mean? Can you be more explicit? We should probably mention that these are non-retryable? src/zookeeper/detector.cpp <https://reviews.apache.org/r/16291/#comment58373> Option<Error> src/zookeeper/detector.cpp <https://reviews.apache.org/r/16291/#comment58377> Is this still supposed to start over when a failure is encountered? Looks like this will spin against Group::watch returning failures immediately. src/zookeeper/detector.cpp <https://reviews.apache.org/r/16291/#comment58379> CHECK not discarded directly. I'm also not sure that mentioning Group is the right thing to do here given we can't accurately attribute the discard to Group. src/zookeeper/detector.cpp <https://reviews.apache.org/r/16291/#comment58383> Looks like this logic be simplified now that you're using an Option: if (current != leader) { foreach (promise, promises) { promise->set(current); } promises.clear(); } src/zookeeper/detector.cpp <https://reviews.apache.org/r/16291/#comment58381> Does this log message make sense? This case looks to be more aptly described as there being no masters detected? - Ben Mahler 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 > >
