mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bernd Mathiske <be...@mesosphere.io>
Subject Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.
Date Fri, 04 Mar 2016 13:29:36 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.
> 
> Joseph Wu wrote:
>     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.

The way I see it we need exactly one constructor and that can be the non-trivial one. The
objective is that all members can be const then. 

I don't mind a large number of temp variables in a local scope.

That said, I would agree with leaving the code as is for now, because it simplifies the current
patch and does not introduce a second kind of change. However, I would recommend an extra
patch on top of that for later.


> 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.
> 
> Joseph Wu wrote:
>     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(...
>     ```

Dropping for same reason as above. Refactoring and improving the programming style can be
dojne in 2 steps instead of one. Let's stick to the refactoring for now.


> 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.
> 
> Joseph Wu wrote:
>     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.

Thanks for checking this! We'll have to readdress this issue at some point, but you did the
right thing here given the circumstances then.


- Bernd


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