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