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