Mailing-List: contact mesos-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: mesos-dev@incubator.apache.org
Content-Type: multipart/alternative;
 boundary="===============1312511837426498641=="
MIME-Version: 1.0
Subject: Re: Review Request: Refactored and simplified the ZooKeeper test
 fixture(s)
 and tests.
From: "Benjamin Hindman" <benh@berkeley.edu>
To: "Vinod Kone" <vinodkone@gmail.com>,
 "Ben Mahler" <benjamin.mahler@gmail.com>
Cc: "mesos" <mesos-dev@incubator.apache.org>,
 "Benjamin Hindman" <benh@berkeley.edu>
Date: Fri, 26 Oct 2012 17:31:38 -0000
Message-ID: <20121026173138.27001.45584@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Benjamin Hindman" <noreply@reviews.apache.org>
References: <20121025174550.28433.50548@reviews.apache.org>
In-Reply-To: <20121025174550.28433.50548@reviews.apache.org>
Reply-To: "Benjamin Hindman" <benh@berkeley.edu>

--===============1312511837426498641==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable


> 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=3D179333#file179333line=
368>
> >
> >     What was happening in these tests when there weren't any calls to M=
asterDetector::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=3D179341#file179341line=
21>
> >
> >     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 h=
eader already included most of these guys.

Yes, C headers first. And yes, treat each file individually. It is way easi=
er 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=3D179341#file179341line=
43>
> >
> >     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 i=
t'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 bd408c22df5edc3684791e509ce1a4c=
210553922 =

>   src/tests/base_zookeeper_test.hpp 06da416d79beb8cd5cf3fa8d102be973b8feb=
4b6 =

>   src/tests/base_zookeeper_test.cpp a188332dd9703ffadbfb2844a9d1036367520=
3a9 =

>   src/tests/state_tests.cpp 0b4aac28f087ad4032fb37f536fb90cd0649e500 =

>   src/tests/zookeeper_server.hpp 6355e8479a636c889945eead12d863b827d78929 =

>   src/tests/zookeeper_server.cpp 817665c14c656b5cdcf930652aab46128839eaf2 =

>   src/tests/zookeeper_server_tests.cpp 7585fa6b837aab60aed0367bdd7582248b=
fb5a21 =

>   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
> =

>


--===============1312511837426498641==--