> On March 9, 2016, 10:46 a.m., Michael Park wrote: > > src/tests/cluster.hpp, line 88 > > <https://reviews.apache.org/r/43613/diff/8/?file=1278081#file1278081line88> > > > > Why do we need to return `Try<process::Owned<Master>>` as opposed to > > `Try<Master>`? > > Joseph Wu wrote: > We return an `Owned` so that the tests can use `.reset()` to destruct the > master/agents. If we just had a `Try<...>`, the tests would have to rely on > scope to destruct, which could get ugly if you have combinations of > masters/agents in a test :) > > i.e. > ``` > Try<Owned<cluster::Master>> master = StartMaster(); > Try<Owned<cluster::Slave>> slave = StartSlave(...); > > // Do stuff. > > master->reset(); > slave->reset(); > > // More stuff > ``` > Vs. > ``` > { > // Have to construct slave after master... > Try<Owned<cluster::Slave>> slave; > > { > Try<Owned<cluster::Master>> master = StartMaster(); > slave = StartSlave(...); > > // Do stuff. > } > } > > // More stuff. > ``` > > Michael Park wrote: > I think your second example you meant to leave out the `Owned`, right? > Assuming that is the case, I would consider using `Try<Option<Master>>` > since > it provides the same capabilities - unnecessary dynamic allocations.
Oops, yes, I meant to leave out the `Owned` in the second part :) > On March 9, 2016, 10:46 a.m., Michael Park wrote: > > src/tests/cluster.hpp, line 108 > > <https://reviews.apache.org/r/43613/diff/8/?file=1278081#file1278081line108> > > > > We're storing a `process::Owned<MasterDetector>`, but returning an > > instance of `process::Owned<MasterDetector>` here. This probably only works > > because `Owned` is implemented as `Shared` currently. This should either be > > `Shared`, or something like `const MasterDetector&`/`const MasterDetector*`. > > Joseph Wu wrote: > Each call to `detector()` returns a new instance of a MasterDetector; the > comment above this line should say the same thing. (Am I interpretting your > issue correctly?) > > This is mostly a convenience function so that you don't see something > like this in all the tests: > ``` > // For the non-zookeeper case: > Owned<MasterDetector> detector(new > StandaloneMasterDetector(master.get()->pid); > ``` > > Michael Park wrote: > I think my source of confusion is that I thought `detector()` simply > returns `masterDetector` > (since I only looked through through the header file). I understand that > is not the case. > I'm currently confused as to what `Master::masterDetector` does, > considering your comment below, which reads: > > Now that `Master` and `Slave` live at the test level, all > `MasterDetector` objects are owned in the test scope. > (If `MasterDetector` is always owned by the test, this eliminates the > need for two `StartSlave` varieties for > `MasterDetector*` and `Owned<MasterDetector>`.) Renamed `detector()` -> `createDetector`. And `masterDetector` -> `detector`. > On March 9, 2016, 10:46 a.m., Michael Park wrote: > > src/tests/cluster.hpp, line 141 > > <https://reviews.apache.org/r/43613/diff/8/?file=1278081#file1278081line141> > > > > The zookeeper setup is for multi-master setup, right? Before, we had a > > zookeeper url per-`Masters`, now we have it per-`Master`. Is the idea to > > simply store duplicates instead? > > Joseph Wu wrote: > If we had multi-master tests, this would store duplicates (we also store > it in MesosTest). (Maybe it will be useful in future to store different > zookeeper URLs per master?) We currently don't support multi-master tests, > and I'm guessing we will tweak zookeeper variables when we move to support > multi-master tests. > > Michael Park wrote: > We don't currently have any multi-master tests? So we don't use > `zookeeperUrl` at all? See: https://issues.apache.org/jira/browse/MESOS-2976 and related/linked JIRAs. - Joseph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43613/#review122760 ----------------------------------------------------------- On March 10, 2016, 12:28 p.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43613/ > ----------------------------------------------------------- > > (Updated March 10, 2016, 12:28 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 > >
