> On Oct. 28, 2013, 5:20 a.m., Vinod Kone wrote: > > src/zookeeper/contender.cpp, lines 190-193 > > <https://reviews.apache.org/r/13086/diff/8/?file=371404#file371404line190> > > > > How is watching.isSome() possible? > > > > I am also not sure how withdrawing.isSome() is possible.
withdrawing.isSome() when the user calls withdraw after calling contending() and before joined() is called. I removed "watching.isSome()" as this was no longer possible. In the past it was the complication of reusability of the contender. > On Oct. 28, 2013, 5:20 a.m., Vinod Kone wrote: > > src/zookeeper/group.cpp, lines 233-235 > > <https://reviews.apache.org/r/13086/diff/8/?file=371408#file371408line233> > > > > why do we need a timer on join? There were added because the client could be calling join() after we have locally timed out the session and recreated the ZK handle but it still cannot get connected. In this case there is no running timer because it already ran and timed out. As a result the operation is pending forever unless the Group reconnects. With the added timeouts all operations fail if Group cannot reconnect to ZK within the local timeout. I now have killed all these after communicating with Vinod offline and in such a case we just let the operations pend. > On Oct. 28, 2013, 5:20 a.m., Vinod Kone wrote: > > src/zookeeper/group.cpp, line 497 > > <https://reviews.apache.org/r/13086/diff/8/?file=371408#file371408line497> > > > > I might've asked this earlier, but why abort() instead of expired()? > > iiuc, the time out is just fast forwarding the expiration process, no? timeout is fast forwarding the expiration process; it does not fail pending operations. In fact, pending operation can still be executed after Group reconnects. Here the goal is to hide the expiration. abort() fails all pending operations and expires the session. Here we want to the inform the clients that we are locally terminating the session (aborting), we do not hide it because the client might not want to wait indefinitely. > On Oct. 28, 2013, 5:20 a.m., Vinod Kone wrote: > > src/zookeeper/group.cpp, line 869 > > <https://reviews.apache.org/r/13086/diff/8/?file=371408#file371408line869> > > > > Why this reset here? Because after this abort/reset, the group is expected to be reused (even before it reconnects, in which case the operations are queued). Resetting the error allows Group operations (join, withdraw, cancel) to be callable again. > On Oct. 28, 2013, 5:20 a.m., Vinod Kone wrote: > > src/zookeeper/contender.cpp, line 217 > > <https://reviews.apache.org/r/13086/diff/8/?file=371404#file371404line217> > > > > 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? We discard memberships in abort() > On Oct. 28, 2013, 5:20 a.m., Vinod Kone wrote: > > src/zookeeper/group.cpp, lines 322-324 > > <https://reviews.apache.org/r/13086/diff/8/?file=371408#file371408line322> > > > > Why here? Comments above. > On Oct. 28, 2013, 5:20 a.m., Vinod Kone wrote: > > src/zookeeper/contender.cpp, line 201 > > <https://reviews.apache.org/r/13086/diff/8/?file=371404#file371404line201> > > > > Why do you even need to store 'membership' when you can use 'candidacy'? It was meant for clarity but seemed not achieving the goal... Killed 'membership'. > On Oct. 28, 2013, 5:20 a.m., Vinod Kone wrote: > > src/zookeeper/contender.cpp, lines 152-153 > > <https://reviews.apache.org/r/13086/diff/8/?file=371404#file371404line152> > > > > Shouldn't we eventually withdraw after contending succeeds? Made it return a failure in such case: if (contending.isNone() || !candidacy.isReady()) { return Future<bool>::failed( "Can only withdraw after the contender has contended"); } - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13086/#review27586 ----------------------------------------------------------- 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 > >
