You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2012/10/24 06:45:21 UTC
Review Request: Refactored and simplified the ZooKeeper test fixture(s) and
tests. In particular:
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7713/
-----------------------------------------------------------
Review request for mesos, Vinod Kone and Ben Mahler.
Description
-------
See summary.
Diffs
-----
src/Makefile.am cf9364ea0418ec30a75b1774f32bda8de6e031ac
src/jvm/jvm.hpp c3ec99248ccb66c1803da2c102ff9be0fb8ddd17
src/sched/sched.cpp 5a4e594a3a8a2089cf74e7e418f2e7bde4c03e9d
src/tests/allocator_zookeeper_tests.cpp bd408c22df5edc3684791e509ce1a4c210553922
src/tests/base_zookeeper_test.hpp 06da416d79beb8cd5cf3fa8d102be973b8feb4b6
src/tests/base_zookeeper_test.cpp a188332dd9703ffadbfb2844a9d10363675203a9
src/tests/state_tests.cpp 0b4aac28f087ad4032fb37f536fb90cd0649e500
src/tests/zookeeper_server.hpp 6355e8479a636c889945eead12d863b827d78929
src/tests/zookeeper_server.cpp 817665c14c656b5cdcf930652aab46128839eaf2
src/tests/zookeeper_server_tests.cpp 7585fa6b837aab60aed0367bdd7582248bfb5a21
src/tests/zookeeper_test.hpp PRE-CREATION
src/tests/zookeeper_test.cpp PRE-CREATION
src/tests/zookeeper_test_server.hpp PRE-CREATION
src/tests/zookeeper_test_server.cpp PRE-CREATION
src/tests/zookeeper_tests.cpp 4415a33b94dd6ca360a7dd3ca49f4c29ee25f5e8
Diff: https://reviews.apache.org/r/7713/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request: Refactored and simplified the ZooKeeper test fixture(s)
and tests. In particular:
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7713/
-----------------------------------------------------------
(Updated Oct. 26, 2012, 6:11 p.m.)
Review request for mesos, Vinod Kone and Ben Mahler.
Changes
-------
Updates from review comments.
Summary (updated)
-----------------
Refactored and simplified the ZooKeeper test fixture(s) and tests. In particular:
Description
-------
In particular:
(a) Eliminated the verbose logging coming from the
AllocatorZooKeeperTest tests that was causing a lot of output when
the tests ran.
(b) Made sure all the ZooKeeper clients are properly shutdown in the
AllocatorZooKeeperTest tests (this was mainly just invoking
MasterDetector::destroy on all created detectors).
(c) Renamed ZooKeeperServer to ZooKeeperServerTest so as not to
conflate it with the Java class
org.apache.zookeeper.server.ZooKeeperServer.
(d) Added an AssertZKGet helper to get better error messages.
Updated BaseZooKeeperTest fixture to shutdown embedeed JVM after the
tests have completed.
Diffs (updated)
-----
src/Makefile.am cf9364ea0418ec30a75b1774f32bda8de6e031ac
src/jvm/jvm.hpp c3ec99248ccb66c1803da2c102ff9be0fb8ddd17
src/sched/sched.cpp 5a4e594a3a8a2089cf74e7e418f2e7bde4c03e9d
src/tests/allocator_zookeeper_tests.cpp bd408c22df5edc3684791e509ce1a4c210553922
src/tests/base_zookeeper_test.hpp 06da416d79beb8cd5cf3fa8d102be973b8feb4b6
src/tests/base_zookeeper_test.cpp a188332dd9703ffadbfb2844a9d10363675203a9
src/tests/state_tests.cpp 0b4aac28f087ad4032fb37f536fb90cd0649e500
src/tests/zookeeper_server.hpp 6355e8479a636c889945eead12d863b827d78929
src/tests/zookeeper_server.cpp 817665c14c656b5cdcf930652aab46128839eaf2
src/tests/zookeeper_server_tests.cpp 7585fa6b837aab60aed0367bdd7582248bfb5a21
src/tests/zookeeper_test.hpp PRE-CREATION
src/tests/zookeeper_test.cpp PRE-CREATION
src/tests/zookeeper_test_server.hpp PRE-CREATION
src/tests/zookeeper_test_server.cpp PRE-CREATION
src/tests/zookeeper_tests.cpp 4415a33b94dd6ca360a7dd3ca49f4c29ee25f5e8
Diff: https://reviews.apache.org/r/7713/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request: Refactored and simplified the ZooKeeper test fixture(s)
and tests.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Oct. 25, 2012, 5:45 p.m., Ben Mahler wrote:
> > src/tests/allocator_zookeeper_tests.cpp, line 368
> > <https://reviews.apache.org/r/7713/diff/1/?file=179333#file179333line368>
> >
> > What was happening in these tests when there weren't any calls to MasterDetector::destroy?
The detector just keept running, and kept trying to connect to ZK.
> On Oct. 25, 2012, 5:45 p.m., Ben Mahler wrote:
> > src/tests/zookeeper_test.cpp, line 21
> > <https://reviews.apache.org/r/7713/diff/1/?file=179341#file179341line21>
> >
> > Why do we put gtest here rather than right before stout?
> > Is it because it's a c header?
> >
> > Also, do we typically do redundant includes here? Considering the header already included most of these guys.
Yes, C headers first. And yes, treat each file individually. It is way easier to reason about.
> On Oct. 25, 2012, 5:45 p.m., Ben Mahler wrote:
> > src/tests/zookeeper_test.cpp, line 43
> > <https://reviews.apache.org/r/7713/diff/1/?file=179341#file179341line43>
> >
> > Comment what this variable represents?
It's commented in the header. Whenever you see a static storage assignment like that, always go look in the header first. (And to be clear, you know it's static because of the 'ZooKeeperTest::').
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7713/#review12772
-----------------------------------------------------------
On Oct. 24, 2012, 4:46 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7713/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2012, 4:46 a.m.)
>
>
> Review request for mesos, Vinod Kone and Ben Mahler.
>
>
> Description
> -------
>
> In particular:
>
> (a) Eliminated the verbose logging coming from the
> AllocatorZooKeeperTest tests that was causing a lot of output when
> the tests ran.
>
> (b) Made sure all the ZooKeeper clients are properly shutdown in the
> AllocatorZooKeeperTest tests (this was mainly just invoking
> MasterDetector::destroy on all created detectors).
>
> (c) Renamed ZooKeeperServer to ZooKeeperServerTest so as not to
> conflate it with the Java class
> org.apache.zookeeper.server.ZooKeeperServer.
>
> (d) Added an AssertZKGet helper to get better error messages.
>
> Updated BaseZooKeeperTest fixture to shutdown embedeed JVM after the
> tests have completed.
>
>
> Diffs
> -----
>
> src/Makefile.am cf9364ea0418ec30a75b1774f32bda8de6e031ac
> src/jvm/jvm.hpp c3ec99248ccb66c1803da2c102ff9be0fb8ddd17
> src/sched/sched.cpp 5a4e594a3a8a2089cf74e7e418f2e7bde4c03e9d
> src/tests/allocator_zookeeper_tests.cpp bd408c22df5edc3684791e509ce1a4c210553922
> src/tests/base_zookeeper_test.hpp 06da416d79beb8cd5cf3fa8d102be973b8feb4b6
> src/tests/base_zookeeper_test.cpp a188332dd9703ffadbfb2844a9d10363675203a9
> src/tests/state_tests.cpp 0b4aac28f087ad4032fb37f536fb90cd0649e500
> src/tests/zookeeper_server.hpp 6355e8479a636c889945eead12d863b827d78929
> src/tests/zookeeper_server.cpp 817665c14c656b5cdcf930652aab46128839eaf2
> src/tests/zookeeper_server_tests.cpp 7585fa6b837aab60aed0367bdd7582248bfb5a21
> src/tests/zookeeper_test.hpp PRE-CREATION
> src/tests/zookeeper_test.cpp PRE-CREATION
> src/tests/zookeeper_test_server.hpp PRE-CREATION
> src/tests/zookeeper_test_server.cpp PRE-CREATION
> src/tests/zookeeper_tests.cpp 4415a33b94dd6ca360a7dd3ca49f4c29ee25f5e8
>
> Diff: https://reviews.apache.org/r/7713/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Refactored and simplified the ZooKeeper test fixture(s)
and tests.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7713/#review12772
-----------------------------------------------------------
Ship it!
cosmetics
src/tests/allocator_zookeeper_tests.cpp
<https://reviews.apache.org/r/7713/#comment27323>
What was happening in these tests when there weren't any calls to MasterDetector::destroy?
src/tests/zookeeper_test.hpp
<https://reviews.apache.org/r/7713/#comment27324>
speaking of wrapping.. let's get this one
src/tests/zookeeper_test.cpp
<https://reviews.apache.org/r/7713/#comment27325>
Why do we put gtest here rather than right before stout?
Is it because it's a c header?
Also, do we typically do redundant includes here? Considering the header already included most of these guys.
src/tests/zookeeper_test.cpp
<https://reviews.apache.org/r/7713/#comment27326>
Comment what this variable represents?
src/tests/zookeeper_test.cpp
<https://reviews.apache.org/r/7713/#comment27327>
wrapping
src/tests/zookeeper_test.cpp
<https://reviews.apache.org/r/7713/#comment27328>
wrapping
src/tests/zookeeper_test.cpp
<https://reviews.apache.org/r/7713/#comment27330>
wrapping
src/tests/zookeeper_test_server.cpp
<https://reviews.apache.org/r/7713/#comment27331>
want to fix the wrapping in this function?
src/tests/zookeeper_test_server.cpp
<https://reviews.apache.org/r/7713/#comment27332>
ditto
- Ben Mahler
On Oct. 24, 2012, 4:46 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7713/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2012, 4:46 a.m.)
>
>
> Review request for mesos, Vinod Kone and Ben Mahler.
>
>
> Description
> -------
>
> In particular:
>
> (a) Eliminated the verbose logging coming from the
> AllocatorZooKeeperTest tests that was causing a lot of output when
> the tests ran.
>
> (b) Made sure all the ZooKeeper clients are properly shutdown in the
> AllocatorZooKeeperTest tests (this was mainly just invoking
> MasterDetector::destroy on all created detectors).
>
> (c) Renamed ZooKeeperServer to ZooKeeperServerTest so as not to
> conflate it with the Java class
> org.apache.zookeeper.server.ZooKeeperServer.
>
> (d) Added an AssertZKGet helper to get better error messages.
>
> Updated BaseZooKeeperTest fixture to shutdown embedeed JVM after the
> tests have completed.
>
>
> Diffs
> -----
>
> src/Makefile.am cf9364ea0418ec30a75b1774f32bda8de6e031ac
> src/jvm/jvm.hpp c3ec99248ccb66c1803da2c102ff9be0fb8ddd17
> src/sched/sched.cpp 5a4e594a3a8a2089cf74e7e418f2e7bde4c03e9d
> src/tests/allocator_zookeeper_tests.cpp bd408c22df5edc3684791e509ce1a4c210553922
> src/tests/base_zookeeper_test.hpp 06da416d79beb8cd5cf3fa8d102be973b8feb4b6
> src/tests/base_zookeeper_test.cpp a188332dd9703ffadbfb2844a9d10363675203a9
> src/tests/state_tests.cpp 0b4aac28f087ad4032fb37f536fb90cd0649e500
> src/tests/zookeeper_server.hpp 6355e8479a636c889945eead12d863b827d78929
> src/tests/zookeeper_server.cpp 817665c14c656b5cdcf930652aab46128839eaf2
> src/tests/zookeeper_server_tests.cpp 7585fa6b837aab60aed0367bdd7582248bfb5a21
> src/tests/zookeeper_test.hpp PRE-CREATION
> src/tests/zookeeper_test.cpp PRE-CREATION
> src/tests/zookeeper_test_server.hpp PRE-CREATION
> src/tests/zookeeper_test_server.cpp PRE-CREATION
> src/tests/zookeeper_tests.cpp 4415a33b94dd6ca360a7dd3ca49f4c29ee25f5e8
>
> Diff: https://reviews.apache.org/r/7713/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Refactored and simplified the ZooKeeper test fixture(s)
and tests.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7713/#review12728
-----------------------------------------------------------
src/tests/zookeeper_tests.cpp
<https://reviews.apache.org/r/7713/#comment27133>
I think this should be &expected, for it to be intuitive.
I like the semantics of
ASSERT_ZK_GET(..,&zk,..) when zk is created on stack,
and ASSERT_ZK_GET(...,zk...) when zk is on heap.
- Vinod Kone
On Oct. 24, 2012, 4:46 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7713/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2012, 4:46 a.m.)
>
>
> Review request for mesos, Vinod Kone and Ben Mahler.
>
>
> Description
> -------
>
> In particular:
>
> (a) Eliminated the verbose logging coming from the
> AllocatorZooKeeperTest tests that was causing a lot of output when
> the tests ran.
>
> (b) Made sure all the ZooKeeper clients are properly shutdown in the
> AllocatorZooKeeperTest tests (this was mainly just invoking
> MasterDetector::destroy on all created detectors).
>
> (c) Renamed ZooKeeperServer to ZooKeeperServerTest so as not to
> conflate it with the Java class
> org.apache.zookeeper.server.ZooKeeperServer.
>
> (d) Added an AssertZKGet helper to get better error messages.
>
> Updated BaseZooKeeperTest fixture to shutdown embedeed JVM after the
> tests have completed.
>
>
> Diffs
> -----
>
> src/Makefile.am cf9364ea0418ec30a75b1774f32bda8de6e031ac
> src/jvm/jvm.hpp c3ec99248ccb66c1803da2c102ff9be0fb8ddd17
> src/sched/sched.cpp 5a4e594a3a8a2089cf74e7e418f2e7bde4c03e9d
> src/tests/allocator_zookeeper_tests.cpp bd408c22df5edc3684791e509ce1a4c210553922
> src/tests/base_zookeeper_test.hpp 06da416d79beb8cd5cf3fa8d102be973b8feb4b6
> src/tests/base_zookeeper_test.cpp a188332dd9703ffadbfb2844a9d10363675203a9
> src/tests/state_tests.cpp 0b4aac28f087ad4032fb37f536fb90cd0649e500
> src/tests/zookeeper_server.hpp 6355e8479a636c889945eead12d863b827d78929
> src/tests/zookeeper_server.cpp 817665c14c656b5cdcf930652aab46128839eaf2
> src/tests/zookeeper_server_tests.cpp 7585fa6b837aab60aed0367bdd7582248bfb5a21
> src/tests/zookeeper_test.hpp PRE-CREATION
> src/tests/zookeeper_test.cpp PRE-CREATION
> src/tests/zookeeper_test_server.hpp PRE-CREATION
> src/tests/zookeeper_test_server.cpp PRE-CREATION
> src/tests/zookeeper_tests.cpp 4415a33b94dd6ca360a7dd3ca49f4c29ee25f5e8
>
> Diff: https://reviews.apache.org/r/7713/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Refactored and simplified the ZooKeeper test fixture(s)
and tests.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7713/
-----------------------------------------------------------
(Updated Oct. 24, 2012, 4:46 a.m.)
Review request for mesos, Vinod Kone and Ben Mahler.
Summary (updated)
-----------------
Refactored and simplified the ZooKeeper test fixture(s) and tests.
Description (updated)
-------
In particular:
(a) Eliminated the verbose logging coming from the
AllocatorZooKeeperTest tests that was causing a lot of output when
the tests ran.
(b) Made sure all the ZooKeeper clients are properly shutdown in the
AllocatorZooKeeperTest tests (this was mainly just invoking
MasterDetector::destroy on all created detectors).
(c) Renamed ZooKeeperServer to ZooKeeperServerTest so as not to
conflate it with the Java class
org.apache.zookeeper.server.ZooKeeperServer.
(d) Added an AssertZKGet helper to get better error messages.
Updated BaseZooKeeperTest fixture to shutdown embedeed JVM after the
tests have completed.
Diffs
-----
src/Makefile.am cf9364ea0418ec30a75b1774f32bda8de6e031ac
src/jvm/jvm.hpp c3ec99248ccb66c1803da2c102ff9be0fb8ddd17
src/sched/sched.cpp 5a4e594a3a8a2089cf74e7e418f2e7bde4c03e9d
src/tests/allocator_zookeeper_tests.cpp bd408c22df5edc3684791e509ce1a4c210553922
src/tests/base_zookeeper_test.hpp 06da416d79beb8cd5cf3fa8d102be973b8feb4b6
src/tests/base_zookeeper_test.cpp a188332dd9703ffadbfb2844a9d10363675203a9
src/tests/state_tests.cpp 0b4aac28f087ad4032fb37f536fb90cd0649e500
src/tests/zookeeper_server.hpp 6355e8479a636c889945eead12d863b827d78929
src/tests/zookeeper_server.cpp 817665c14c656b5cdcf930652aab46128839eaf2
src/tests/zookeeper_server_tests.cpp 7585fa6b837aab60aed0367bdd7582248bfb5a21
src/tests/zookeeper_test.hpp PRE-CREATION
src/tests/zookeeper_test.cpp PRE-CREATION
src/tests/zookeeper_test_server.hpp PRE-CREATION
src/tests/zookeeper_test_server.cpp PRE-CREATION
src/tests/zookeeper_tests.cpp 4415a33b94dd6ca360a7dd3ca49f4c29ee25f5e8
Diff: https://reviews.apache.org/r/7713/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman