> 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>`?

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


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

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


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

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.


> On March 9, 2016, 10:46 a.m., Michael Park wrote:
> > src/tests/cluster.hpp, line 142
> > <https://reviews.apache.org/r/43613/diff/8/?file=1278081#file1278081line142>
> >
> >     `Files` used to be at the cluster level, now it is at a per-`Master` 
> > level. Could you explain briefly what this is used for and why it's ok to 
> > do this?

`Files` is also at a per-`Slave` level now :)

The `Files` is mostly a dependency that doesn't play a big role in the tests.  
Besides `files_test.cpp`, I believe tests don't use `Files` at all.
(All it does is expose some files on disk via an HTTP endpoint.)

It's safe to have multiple instances of `Files` because of the above reason, 
and because `Files` is read-only.


> On March 9, 2016, 10:46 a.m., Michael Park wrote:
> > src/tests/cluster.hpp, lines 236-237
> > <https://reviews.apache.org/r/43613/diff/8/?file=1278081#file1278081line236>
> >
> >     Could you explain why this used to be owned, but now is non-owned?

The `MasterDetector` used to be populated by the `Cluster` object, because the 
`Cluster` held a `Master` object.  When the `MasterDetector` was made in that 
way, the `Slave` would own the detector.

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

Note: Technically, we don't need to store this pointer in the `Slave` at the 
moment.  Compared to `Containerizer* containerizer;` which is used in the 
`Slave` destructor, the `detector` is merely passed into the `slave::Slave` 
object and left alone afterwards.


- Joseph


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


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