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 Wed, 09 Mar 2016 20:08:35 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>`?

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


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