----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13086/#review28588 -----------------------------------------------------------
Looks good, I mostly just had a question about the semantics in LeaderContender being different between discards and withdrawal. src/zookeeper/contender.hpp <https://reviews.apache.org/r/13086/#comment55427> Is there a difference between withdraw() and calling discard() on one of the Futures returned by contend? The implementation of LeaderContender appears to do different things depending on whether the contend was discarded or withdraw() was called. Is it possible to simplify this and have both operations go through the same code paths? It seems like withdraw can handle all possible valid withdrawal states and the onDiscarded callbacks can call withdraw instead of fail? Or is there a reason not to withdraw when the client discards the future? src/zookeeper/contender.hpp <https://reviews.apache.org/r/13086/#comment55444> Seems like calling withdraw twice is harmless? Should we just return true instead of a failed Future? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment55432> Let's CHECK here instead since the caller is violating the intended usage. { CHECK(contending.isNone()) << "Cannot contend more than once"; ... } src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment55436> '" << data << "'"; src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment55435> If candidacy is not ready, it's still possible that contend() has been called (if the group->join is still in progress). This doesn't match the documentation in contender.hpp: // Returns true if successfully withdrawn from the contest (either // while contending or has already contended and is watching for // membership loss). // A false return value implies that there was no valid group // membership to cancel, which may be a result of a race to cancel // an expired membership. // A failed future is returned if the method is called before // contend() or more than once. process::Future<bool> withdraw(); In the case where contend() has been called, and withdraw() was called prior to us joining the Group, we'll be returning a failed Future? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment55441> Looks like you're not expecting memberships to be discarded, mind adding a CHECK for this? src/zookeeper/detector.hpp <https://reviews.apache.org/r/13086/#comment55447> s/a "leader"/the leader/ I wonder if we need the quotes around "leader" here and in contender? Seems to leave an open question around the meaning of leader. src/zookeeper/detector.hpp <https://reviews.apache.org/r/13086/#comment55448> Would love to see a todo here: TODO: Use a Stream abstraction instead. src/zookeeper/detector.cpp <https://reviews.apache.org/r/13086/#comment55449> Why was this needed? There's a typedef for this in Process. Should we be using Self in the contender as well? src/zookeeper/detector.cpp <https://reviews.apache.org/r/13086/#comment55465> How about making this a queue so we can pop off promises when we set them, akin to what is done in group.cpp? src/zookeeper/detector.cpp <https://reviews.apache.org/r/13086/#comment55450> Is the eager watch simpler than watching lazily? src/zookeeper/detector.cpp <https://reviews.apache.org/r/13086/#comment55454> My brain hurts ;) How about: if (leader.isError() != previous.isError() || leader.isNone() != previous.isNone() || leader.isSome() != previous.isSome()) { return leader; // State change. } else if (leader.isSome() && previous.isSome() && leader.get() != previous.get()) { return leader; // Leadership change. } src/zookeeper/detector.cpp <https://reviews.apache.org/r/13086/#comment55466> "was lost" src/zookeeper/detector.cpp <https://reviews.apache.org/r/13086/#comment55467> Does s/Result<Group::Membership>::none()/None()/ work? - Ben Mahler On Nov. 5, 2013, 10:17 p.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/13086/ > ----------------------------------------------------------- > > (Updated Nov. 5, 2013, 10:17 p.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone. > > > Bugs: MESOS-496 > https://issues.apache.org/jira/browse/MESOS-496 > > > Repository: mesos-git > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/Makefile.am 9780d07a23ca196c541a44a85499c2f44a574b9c > src/tests/zookeeper_test_server.cpp > 0b22f319a53d08f9fe15c97c634b03358f40ba7e > src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce > src/zookeeper/contender.hpp PRE-CREATION > src/zookeeper/contender.cpp PRE-CREATION > src/zookeeper/detector.hpp PRE-CREATION > src/zookeeper/detector.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/13086/diff/ > > > Testing > ------- > > make check 100 times. > > > Thanks, > > Jiang Yan Xu > >
