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

Reply via email to