----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16111/#review30048 -----------------------------------------------------------
src/state/zookeeper.cpp <https://reviews.apache.org/r/16111/#comment57576> 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. src/tests/zookeeper.hpp <https://reviews.apache.org/r/16111/#comment57577> 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. src/tests/zookeeper_test_server.hpp <https://reviews.apache.org/r/16111/#comment57575> Should these be const? src/tests/zookeeper_test_server.cpp <https://reviews.apache.org/r/16111/#comment57557> Perhaps a comment here as to how you knew that the int represented milliseconds in the ZK code? src/tests/zookeeper_test_server.cpp <https://reviews.apache.org/r/16111/#comment57559> Is the cast needed on these two? src/tests/zookeeper_tests.cpp <https://reviews.apache.org/r/16111/#comment57574> Thanks for the test! src/zookeeper/group.cpp <https://reviews.apache.org/r/16111/#comment57573> 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. src/zookeeper/zookeeper.hpp <https://reviews.apache.org/r/16111/#comment57570> Maybe s/timeout/sessionTimeout/ here and elsewhere to be more clear. src/zookeeper/zookeeper.cpp <https://reviews.apache.org/r/16111/#comment57571> Is this cast necessary? Looks like we're widening. - Ben Mahler On Dec. 9, 2013, 6:51 p.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16111/ > ----------------------------------------------------------- > > (Updated Dec. 9, 2013, 6:51 p.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, Raul Gutierrez > Segales, 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/state/zookeeper.cpp 09b63d44e9349cab2d73659c939de3d8e96fbcc5 > src/tests/zookeeper.hpp 1bc38c291cef39a4d255fd9065428a26d86248cb > src/tests/zookeeper.cpp 8bb49012d8dc46ef9f5a64ead1654253b9df8c21 > 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/watcher.hpp 1db0386719c2a675d29b47b417dc856993062326 > 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 > >
