> On March 9, 2016, 6:46 p.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.
>     ```

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.


> On March 9, 2016, 6:46 p.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.

We don't currently have any multi-master tests? So we don't use `zookeeperUrl` 
at all?


> On March 9, 2016, 6:46 p.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);
>     ```

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>`.)


- Michael


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


On March 9, 2016, 8:10 p.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> -----------------------------------------------------------
> 
> (Updated March 9, 2016, 8:10 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
> 
>

Reply via email to