> On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote: > > src/zookeeper/contender.cpp, lines 32-33 > > <https://reviews.apache.org/r/13086/diff/6/?file=364281#file364281line32> > > > > what is the difference?
Killed `virtual void initialize()`; > On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote: > > src/zookeeper/contender.cpp, line 42 > > <https://reviews.apache.org/r/13086/diff/6/?file=364281#file364281line42> > > > > how do you know you failed? The future is not passed as a parameter but stored as a class variable. > On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote: > > src/zookeeper/contender.cpp, line 59 > > <https://reviews.apache.org/r/13086/diff/6/?file=364281#file364281line59> > > > > s/abort/cancel/? > > > > or > > > > s/abort/fail/ ? s/abort/fail/ > On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote: > > src/zookeeper/contender.cpp, lines 140-141 > > <https://reviews.apache.org/r/13086/diff/6/?file=364281#file364281line140> > > > > So, if we are already withdrawing we just return the future. Why not do > > the same when we are contending? Ok. > On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote: > > src/zookeeper/contender.cpp, line 177 > > <https://reviews.apache.org/r/13086/diff/6/?file=364281#file364281line177> > > > > s/succ/success/ ? Did s/succ/successful/ > On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote: > > src/zookeeper/detector.cpp, lines 112-149 > > <https://reviews.apache.org/r/13086/diff/6/?file=364283#file364283line112> > > > > 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()); > > > > > On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote: > > src/zookeeper/contender.cpp, line 134 > > <https://reviews.apache.org/r/13086/diff/6/?file=364281#file364281line134> > > > > Is this supposed to be "&&" or "||" ? > > > > What if we started contending but joined() hasn't been called yet? Is > > that a failure? > On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote: > > src/zookeeper/contender.cpp, line 179 > > <https://reviews.apache.org/r/13086/diff/6/?file=364281#file364281line179> > > > > How come you are not checking if "success" is ready/failed/discarded? Because we are funneling the Future, regardless ready/failed/discarded, to the caller waiting on the Future returned by withdraw, which has the same signature. > On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote: > > src/zookeeper/contender.cpp, line 195 > > <https://reviews.apache.org/r/13086/diff/6/?file=364281#file364281line195> > > > > LOG(ERROR) ? Should it? The code here propagates the error through Future::failed() and is not swallowing the error. > On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote: > > src/zookeeper/contender.cpp, lines 204-212 > > <https://reviews.apache.org/r/13086/diff/6/?file=364281#file364281line204> > > > > Can we do this inside cancelled() ? Removed this because we now fail withdraws when contending is ongoing instead of promising that it will be withdrawn after the membership is obtained. > On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote: > > src/zookeeper/contender.cpp, lines 252-255 > > <https://reviews.apache.org/r/13086/diff/6/?file=364281#file364281line252> > > > > 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()); Removed this check. > On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote: > > src/zookeeper/detector.cpp, line 79 > > <https://reviews.apache.org/r/13086/diff/6/?file=364283#file364283line79> > > > > Why not: > > if (leader.isSome() and !previous.isSome()) { > > return leader; > > } Changed to if (leader.isSome() && ( !previous.isSome() || leader.get().id() != previous.get().id())) { return leader; } > On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote: > > src/zookeeper/group.cpp, lines 865-868 > > <https://reviews.apache.org/r/13086/diff/6/?file=364285#file364285line865> > > > > How come we don't do this in expired() and only here? The current semantics is that when the session expires we transparently clean up things and reconnect and Group's clients will start receiving notifications again. The memberships that this Group "owned" are lost but the benefit is that for watching un-owned memberships the expiration could be hidden from the client. - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13086/#review26995 ----------------------------------------------------------- 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 > >
