> On Dec. 9, 2013, 10:46 p.m., Ben Mahler wrote: > > src/tests/zookeeper.hpp, line 88 > > <https://reviews.apache.org/r/16111/diff/1/?file=395515#file395515line88> > > > > timeout is only applicable for session connected events, so maybe this > > makes more sense as an Option<Duration> with a comment that reflects when > > it is Some? (We could of course have different Events but let's keep this > > patch simple :)). > > > > The code that assumes it to be some can do the appropriate CHECK.
This is no longer applicable with the addition of zk->getSessionTimeout() > On Dec. 9, 2013, 10:46 p.m., Ben Mahler wrote: > > src/zookeeper/zookeeper.hpp, line 56 > > <https://reviews.apache.org/r/16111/diff/1/?file=395523#file395523line56> > > > > Maybe s/timeout/sessionTimeout/ here and elsewhere to be more clear. Dropped because the timeout is now directly accessible through ZooKeeper::getSessionTimeout() > On Dec. 9, 2013, 10:46 p.m., Ben Mahler wrote: > > src/zookeeper/zookeeper.cpp, line 375 > > <https://reviews.apache.org/r/16111/diff/1/?file=395524#file395524line375> > > > > Is this cast necessary? Looks like we're widening. Now use implicit casting > On Dec. 9, 2013, 10:46 p.m., Ben Mahler wrote: > > src/zookeeper/group.cpp, lines 291-292 > > <https://reviews.apache.org/r/16111/diff/1/?file=395521#file395521line291> > > > > This also results in us using the negotiated session timeout for the > > subsequent zookeeper connection in expired(), was that intended? If so, > > perhaps mention that here. Now reverted this change and only use zk->getSessoinTimeout when setting up the local reconnect timer. Future ZooKeeper re-instantiations will still use the timeout requested by the client. > On Dec. 9, 2013, 10:46 p.m., Ben Mahler wrote: > > src/tests/zookeeper_test_server.cpp, lines 94-102 > > <https://reviews.apache.org/r/16111/diff/1/?file=395518#file395518line94> > > > > Is the cast needed on these two? Now use implicit casts. > On Dec. 9, 2013, 10:46 p.m., Ben Mahler wrote: > > src/tests/zookeeper_test_server.cpp, line 82 > > <https://reviews.apache.org/r/16111/diff/1/?file=395518#file395518line82> > > > > Perhaps a comment here as to how you knew that the int represented > > milliseconds in the ZK code? Done. > On Dec. 9, 2013, 10:46 p.m., Ben Mahler wrote: > > src/state/zookeeper.cpp, line 214 > > <https://reviews.apache.org/r/16111/diff/1/?file=395514#file395514line214> > > > > Ditto about the subsequent ZK connection using this timeout as well, > > can you add a note if this was what you intended? > > > > Why not just say: > > > > // Update the session timeout to the negotiated value. > > > > Here and elsewhere. This is no longer applicable now that the member variable stays 'const'. - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16111/#review30048 ----------------------------------------------------------- On Dec. 10, 2013, 6:57 p.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16111/ > ----------------------------------------------------------- > > (Updated Dec. 10, 2013, 6:57 p.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone. > > > Bugs: MESOS-868 > https://issues.apache.org/jira/browse/MESOS-868 > > > Repository: mesos-git > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/jvm/org/apache/zookeeper.hpp dac14565a66397f153cdc059859286f8ac555919 > src/state/zookeeper.hpp 90b660737f52df426759877feb979b14ac4b6811 > src/tests/zookeeper_test_server.hpp > 97a8524600fe3a57cf084c0dea8e99e9a056c504 > src/tests/zookeeper_test_server.cpp > dc53d6a182a861544d2f9e7fa873be4c8c402856 > src/tests/zookeeper_tests.cpp a5fe9e18fbaa88ea56662dc1b2e3d51fb0b50822 > src/zookeeper/group.hpp facfb1fe31eeeb042c0e2b94d739101911620cdf > src/zookeeper/group.cpp 5c92c5f89d441b2555d928772fa40573660e3e5a > src/zookeeper/zookeeper.hpp 72435432e433fc0162f8b88e2045efcc42793a3a > src/zookeeper/zookeeper.cpp cc8a7caeedb2c109d4952a6520cc98565adaa700 > > Diff: https://reviews.apache.org/r/16111/diff/ > > > Testing > ------- > > ./bin/mesos-tests.sh > --gtest_filter=GroupTest*:ZooKeeperTest*:ZooKeeperMasterContenderDetectorTest*:ZooKeeperStateTest* > -j --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle > > > Thanks, > > Jiang Yan Xu > >
