> On March 3, 2016, 5:22 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.hpp, line 213
> > <https://reviews.apache.org/r/43613/diff/8/?file=1278081#file1278081line213>
> >
> > I don't think we need this constructor. We are using an instance of
> > this merely as an aggregate temporary variable for the injections into the
> > actual instance to be constructed.
I'd argue the small constructor is better for readability.
The alternative would involve adding one temporary variable for each injection
into the `cluster::Slave::start` factory. The private constructor would then
look like:
```
class Slave
{
...
private:
Slave(
MasterDetector* _detector,
slave::Flags _flags,
slave::Containerizer* _containerizer,
process::Owned<slave::Containerizer>& _ownedContainerizer,
process::Owned<slave::Fetcher>& _fetcher,
process::Owned<slave::GarbageCollector>& _gc,
process::Owned<mesos::slave::QoSController>& _qosController,
process::Owned<mesos::slave::ResourceEstimator>& _resourceEstimator,
process::Owned<slave::StatusUpdateManager>& _statusUpdateManager)
: detector(_detector),
flags(_flags),
containerizer(_containerizer),
ownedContainerizer(_ownedContainerizer),
fetcher(_fetcher),
gc(_gc),
qosController(_qosController),
resourceEstimator(_resourceEstimator),
statusUpdateManager(_statusUpdateManager)
{
slave = new slave::Slave(
id.isSome() ? id.get() : process::ID::generate("slave"),
flags,
detector,
slave->containerizer,
&slave->files,
gc.getOrElse(slave->gc.get()),
statusUpdateManager.getOrElse(slave->statusUpdateManager.get()),
resourceEstimator.getOrElse(slave->resourceEstimator.get()),
qosController.getOrElse(slave->qosController.get()));
}
}
```
That's a lot of code bloat just to pass some variables around.
Also, at least one constructor is necessary so that we can enforce the
`private`-ness of said constructor. It wouldn't make sense to have tests
construct the `cluster::Slave` in any other way.
> On March 3, 2016, 5:22 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.cpp, line 279
> > <https://reviews.apache.org/r/43613/diff/8/?file=1278082#file1278082line279>
> >
> > We shouldn't really use a temp master object just to reset it with the
> > same values it already has. I'd prefer a more functional style with only
> > one constructor. See also the slave below.
We aren't resetting the `cluster::Master` object though. This line is passing
objects owned by `cluster::Master` into `master::Master`.
Would it be more clear if the line read like this?
```
master->master = new master::Master(...
```
> On March 3, 2016, 5:22 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.cpp, line 339
> > <https://reviews.apache.org/r/43613/diff/8/?file=1278082#file1278082line339>
> >
> > Before this patch, with a cluster object that holds all masters, it
> > makes sense to revoke the authenticator for all masters at shutdown.
> > However, in our tests, each master constructor sets the authenticator in
> > libprocess the same way, with an equivalent value. And libprocess assumes
> > ownership, i.e. it will destruct any lingering authenticator eventually. It
> > will also destruct the previous one if a new one is set.
> >
> > Therefore, we don't need to actively unset the authenticator here. In
> > fact, this prevents multi-master tests. If one master gets destructed, it
> > takes the authenticator for the others with it. Because there is only one -
> > in libprocess.
There are a couple other problems with multi-master tests
(https://issues.apache.org/jira/browse/MESOS-2976).
Note that the comment right below was retained from here
https://reviews.apache.org/r/43614/diff/2#1.97
```
// This means that multiple masters in tests are not supported.
```
---
Looks like there are also a few tests that require this.
On OSX, I removed this line and saw these tests fail:
```
[ FAILED ] MasterQuotaTest.NoAuthenticationNoAuthorization
[ FAILED ] MasterQuotaTest.AuthorizeQuotaRequestsWithoutPrincipal
[ FAILED ] PersistentVolumeEndpointsTest.NoAuthentication
[ FAILED ] ReservationEndpointsTest.ReserveAndUnreserveNoAuthentication
```
I'm guessing these tests assume the authenticator is cleaned up in previous
tests.
> On March 3, 2016, 5:22 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.cpp, line 431
> > <https://reviews.apache.org/r/43613/diff/8/?file=1278082#file1278082line431>
> >
> > I suggest we avoid this temporary slave instance and create the real
> > one in one swoop below. This is then closer to functional style.
> >
> > Then there is no need to copy parameters to `slave->...` here either.
See my response about the `cluster::Slave` constructor above. If you feel
that's the proper way of doing things, then I'll change this accordingly.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review121835
-----------------------------------------------------------
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
>
>
