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, 03 Mar 2016 19:06:50 GMT


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


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