----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43613/#review119989 -----------------------------------------------------------
src/tests/cluster.hpp (line 82) <https://reviews.apache.org/r/43613/#comment181349> I'm pondering if calling the factory method `start()` is the right way. People are already used to the `create()` name. In that case we can also add an `start()` public method. Not an issue but an idea. Same goes for slave. src/tests/cluster.hpp (lines 105 - 106) <https://reviews.apache.org/r/43613/#comment181348> I think we are slowly introducing the pattern of using default deleted functions in C++, if you do ```shell ag --cpp --ignore-dir='build' '= ?delete;' . ``` you will find a bunch of instances of it. Seems like a perfect opportunity to do: ```c++ Master(const Master&) = delete; Master& operator=(const Master&) = delete; ``` src/tests/cluster.hpp (line 122) <https://reviews.apache.org/r/43613/#comment181352> Not yours (is the only unchanged line here) but if all other pointer objects are of type `process::Owned` but here it is `std::shared_ptr`. I think we could move towards consistency here and change the type to `process::Owned`. src/tests/cluster.cpp (lines 192 - 193) <https://reviews.apache.org/r/43613/#comment181354> If we stick to `std::shared_ptr` I would suggest to change its contstruction to `std::make_shared` src/tests/cluster.cpp (line 374) <https://reviews.apache.org/r/43613/#comment181357> Given that you are calling in this same function and there's no risk of races at this point, I will recommend using `[this]() {…`. src/tests/cluster.cpp (line 396) <https://reviews.apache.org/r/43613/#comment181356> Comming from a world with exceptions, destructors are not supposed to throw, which makes me feel uneasy about an `ASSERT` here. But feel free to drop. - Alexander Rojas On Feb. 19, 2016, 8:36 p.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43613/ > ----------------------------------------------------------- > > (Updated Feb. 19, 2016, 8:36 p.m.) > > > Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Artem > Harutyunyan. > > > 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 > >
