----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13086/#review26995 -----------------------------------------------------------
I like the detector code. Its pretty simple and straightforward. We should think more about the contender because currently its a bit complex to follow. src/tests/zookeeper_tests.cpp <https://reviews.apache.org/r/13086/#comment52567> this should come before stout headers. src/zookeeper/contender.hpp <https://reviews.apache.org/r/13086/#comment52568> new line. src/zookeeper/contender.hpp <https://reviews.apache.org/r/13086/#comment52569> s/detector/contender/ src/zookeeper/contender.hpp <https://reviews.apache.org/r/13086/#comment52573> I commented about these in the other review. src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52574> s/detector/contender/ src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52579> what is the difference? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52575> how do you know you failed? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52576> s/abort/cancel/? or s/abort/fail/ ? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52577> Comments on what each of these represent would be great! src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52578> Put initializer on the next line? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52643> src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52626> s/Cannot contend because// Considering the caller of this method will/can do "Failed to contend: " << future.failure() src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52580> s/we've/we are/ src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52581> Not really "only once" right? IIUC, a contend()--> withdraw() --> contend() is valid? How about? "Already contending. Withdraw first before re-contending" ? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52640> onAny on new line? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52628> Is this supposed to be "&&" or "||" ? What if we started contending but joined() hasn't been called yet? Is that a failure? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52627> s/Cannot withdraw because we/We/ ? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52632> So, if we are already withdrawing we just return the future. Why not do the same when we are contending? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52636> Not clear what this comment means. Can you clarify? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52633> s/defending// ? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52630> s/succ/success/ ? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52634> How come you are not checking if "success" is ready/failed/discarded? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52631> Do we need the if check? Isn't set going to be a no-op if its already ready/failed? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52635> LOG(ERROR) ? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52638> Can we do this inside cancelled() ? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52637> It seems weird to do it here! Can this be a onAny() callback on contending? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52641> Again. checking the future is pending here is weird. What happens if it goes out of pending just after this check? Also this should be after the CHECK(memberships.isReady()); src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment52642> A log message here would be nice? src/zookeeper/detector.hpp <https://reviews.apache.org/r/13086/#comment52644> s/membership/leader/ src/zookeeper/detector.hpp <https://reviews.apache.org/r/13086/#comment52645> s/specified/previous/ src/zookeeper/detector.cpp <https://reviews.apache.org/r/13086/#comment52646> s/has/have/ src/zookeeper/detector.cpp <https://reviews.apache.org/r/13086/#comment52647> Why "<" instead of "!="? src/zookeeper/detector.cpp <https://reviews.apache.org/r/13086/#comment52648> Why not: if (leader.isSome() and !previous.isSome()) { return leader; } src/zookeeper/detector.cpp <https://reviews.apache.org/r/13086/#comment52649> A log message here would be nice. src/zookeeper/detector.cpp <https://reviews.apache.org/r/13086/#comment52652> How about the following? Option<Group::Membership> current; foreach(const Group::Membership& membership, memberships.get()) { if (current.isNone() || membership.id() < current.get().id() { current = membership; } } if (current.iSome() && (!leader.Some() || current.get() != leader.get()) { // Set all promises to current. } else if (current.isNone() && !leader.isNone()) { // Set all promises to None. } promises.clear(); leader = current; watch(memberships.get()); src/zookeeper/detector.cpp <https://reviews.apache.org/r/13086/#comment52650> s/the// src/zookeeper/group.cpp <https://reviews.apache.org/r/13086/#comment52653> put out on the previous line. src/zookeeper/group.cpp <https://reviews.apache.org/r/13086/#comment52655> How come we don't do this in expired() and only here? src/zookeeper/group.cpp <https://reviews.apache.org/r/13086/#comment52654> s/expires/expire/ - Vinod Kone On Oct. 14, 2013, 11:30 p.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/13086/ > ----------------------------------------------------------- > > (Updated Oct. 14, 2013, 11:30 p.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, > and Vinod Kone. > > > Bugs: MESOS-496 > https://issues.apache.org/jira/browse/MESOS-496 > > > Repository: mesos-git > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/Makefile.am a2d82425f74dcaf3adf28ae8184fff53b3c34ceb > 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 > src/zookeeper/group.hpp 6b6c1557658df1bb7b7df3ded5c203e2dc0c8308 > src/zookeeper/group.cpp cd58d230a2715b9ed01002443ebd7b5366d31be6 > > Diff: https://reviews.apache.org/r/13086/diff/ > > > Testing > ------- > > make check 100 times. > > > Thanks, > > Jiang Yan Xu > >
