----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43613/#review121835 -----------------------------------------------------------
src/tests/cluster.hpp (line 168) <https://reviews.apache.org/r/43613/#comment183674> 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. src/tests/cluster.cpp (line 237) <https://reviews.apache.org/r/43613/#comment183687> 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. src/tests/cluster.cpp (line 294) <https://reviews.apache.org/r/43613/#comment183678> 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. src/tests/cluster.cpp (line 331) <https://reviews.apache.org/r/43613/#comment183675> 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. src/tests/cluster.cpp (line 335) <https://reviews.apache.org/r/43613/#comment183676> These statements can be avoided if we don't use a temp slave var with default constructor. src/tests/cluster.cpp (line 404) <https://reviews.apache.org/r/43613/#comment183677> This could move inside the constructor, so the pid can be a constant. But that's probably an uncommon pattern in Mesos. So OK to leave as is. - Bernd Mathiske 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 > >
