----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13086/#review27586 -----------------------------------------------------------
This is looking good! I think we are getting close to get this committed. Thanks for the iterations. My main comments are: --> I think we can get rid of couple instance variables in contender. --> Lets split the group changes in to their own review. IIUC, the main goal there is to add a time out. src/zookeeper/contender.hpp <https://reviews.apache.org/r/13086/#comment53597> s/Creates a new leader contender instance.// src/zookeeper/contender.hpp <https://reviews.apache.org/r/13086/#comment53598> s/Destroys the contender object.// src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment53599> Kill this statement. src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment53600> Kill this. src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment53602> Why optional? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment53601> How about "contend() can only be called once." ? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment53604> Shouldn't we eventually withdraw after contending succeeds? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment53606> LOG? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment53607> How is watching.isSome() possible? I am also not sure how withdrawing.isSome() is possible. src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment53610> Why do you even need to store 'membership' when you can use 'candidacy'? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment53612> Why do you need to store 'memberships'. It looks like it is only used in watched(). So can't we just pass it via onAny? src/zookeeper/contender.cpp <https://reviews.apache.org/r/13086/#comment53611> LOG? src/zookeeper/detector.hpp <https://reviews.apache.org/r/13086/#comment53613> s/Creates a new leader detector instance.// src/zookeeper/group.cpp <https://reviews.apache.org/r/13086/#comment53614> why do we need a timer on join? src/zookeeper/group.cpp <https://reviews.apache.org/r/13086/#comment53615> Why here? src/zookeeper/group.cpp <https://reviews.apache.org/r/13086/#comment53617> I might've asked this earlier, but why abort() instead of expired()? iiuc, the time out is just fast forwarding the expiration process, no? src/zookeeper/group.cpp <https://reviews.apache.org/r/13086/#comment53616> Why this reset here? - Vinod Kone On Oct. 25, 2013, 5:46 p.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/13086/ > ----------------------------------------------------------- > > (Updated Oct. 25, 2013, 5:46 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 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 > >
