mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joseph Wu <jos...@mesosphere.io>
Subject Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.
Date Thu, 10 Mar 2016 20:30:40 GMT


> On March 9, 2016, 10:46 a.m., Michael Park wrote:
> > src/tests/cluster.hpp, line 88
> > <https://reviews.apache.org/r/43613/diff/8/?file=1278081#file1278081line88>
> >
> >     Why do we need to return `Try<process::Owned<Master>>` as opposed
to `Try<Master>`?
> 
> Joseph Wu wrote:
>     We return an `Owned` so that the tests can use `.reset()` to destruct the master/agents.
 If we just had a `Try<...>`, the tests would have to rely on scope to destruct, which
could get ugly if you have combinations of masters/agents in a test :)
>     
>     i.e.
>     ```
>     Try<Owned<cluster::Master>> master = StartMaster();
>     Try<Owned<cluster::Slave>> slave = StartSlave(...);
>     
>     // Do stuff.
>     
>     master->reset();
>     slave->reset();
>     
>     // More stuff
>     ```
>     Vs.
>     ```
>     {
>       // Have to construct slave after master...
>       Try<Owned<cluster::Slave>> slave;
>       
>       {
>         Try<Owned<cluster::Master>> master = StartMaster();
>         slave = StartSlave(...);
>         
>         // Do stuff.
>       }
>     }
>     
>     // More stuff.
>     ```
> 
> Michael Park wrote:
>     I think your second example you meant to leave out the `Owned`, right?
>     Assuming that is the case, I would consider using `Try<Option<Master>>`
since
>     it provides the same capabilities - unnecessary dynamic allocations.

Oops, yes, I meant to leave out the `Owned` in the second part :)


> On March 9, 2016, 10:46 a.m., Michael Park wrote:
> > src/tests/cluster.hpp, line 108
> > <https://reviews.apache.org/r/43613/diff/8/?file=1278081#file1278081line108>
> >
> >     We're storing a `process::Owned<MasterDetector>`, but returning an instance
of `process::Owned<MasterDetector>` here. This probably only works because `Owned` is
implemented as `Shared` currently. This should either be `Shared`, or something like `const
MasterDetector&`/`const MasterDetector*`.
> 
> Joseph Wu wrote:
>     Each call to `detector()` returns a new instance of a MasterDetector; the comment
above this line should say the same thing.  (Am I interpretting your issue correctly?)
>     
>     This is mostly a convenience function so that you don't see something like this in
all the tests:
>     ```
>     // For the non-zookeeper case:
>     Owned<MasterDetector> detector(new StandaloneMasterDetector(master.get()->pid);
>     ```
> 
> Michael Park wrote:
>     I think my source of confusion is that I thought `detector()` simply returns `masterDetector`
>     (since I only looked through through the header file). I understand that is not the
case.
>     I'm currently confused as to what `Master::masterDetector` does, considering your
comment below, which reads:
>     > Now that `Master` and `Slave` live at the test level, all `MasterDetector` objects
are owned in the test scope.
>       (If `MasterDetector` is always owned by the test, this eliminates the need for
two `StartSlave` varieties for
>       `MasterDetector*` and `Owned<MasterDetector>`.)

Renamed `detector()` -> `createDetector`.  And `masterDetector` -> `detector`.


> On March 9, 2016, 10:46 a.m., Michael Park wrote:
> > src/tests/cluster.hpp, line 141
> > <https://reviews.apache.org/r/43613/diff/8/?file=1278081#file1278081line141>
> >
> >     The zookeeper setup is for multi-master setup, right? Before, we had a zookeeper
url per-`Masters`, now we have it per-`Master`. Is the idea to simply store duplicates instead?
> 
> Joseph Wu wrote:
>     If we had multi-master tests, this would store duplicates (we also store it in MesosTest).
 (Maybe it will be useful in future to store different zookeeper URLs per master?)  We currently
don't support multi-master tests, and I'm guessing we will tweak zookeeper variables when
we move to support multi-master tests.
> 
> Michael Park wrote:
>     We don't currently have any multi-master tests? So we don't use `zookeeperUrl` at
all?

See: https://issues.apache.org/jira/browse/MESOS-2976 and related/linked JIRAs.


- Joseph


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review122760
-----------------------------------------------------------


On March 10, 2016, 12:28 p.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> -----------------------------------------------------------
> 
> (Updated March 10, 2016, 12:28 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem Harutyunyan, and Michael
Park.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
>     https://issues.apache.org/jira/browse/MESOS-4633
>     https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope of test objects
to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  The `Slave`
object performs cleanup originally found in `cluster::Slave::stop`.  `cluster::Master::start`
and `cluster::Slave::start` were changed to factory methods.
> 
> 
> Diffs
> -----
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> -------
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message