> On Feb. 19, 2016, 3:46 p.m., Alexander Rojas wrote:
> > src/tests/cluster.hpp, line 163
> > <https://reviews.apache.org/r/43613/diff/5/?file=1263028#file1263028line163>
> >
> > 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`.
Unfortunately, this one is a `shared_ptr` because the underlying `Master`
objects expects it as a `shared_ptr` (I don't know why we break the pattern
here.)
If we want to change the pattern, we would do what the
TestingMesosSchedulerDriver does :)
```
// No-op destructor as _detector lives on the stack.
detector =
std::shared_ptr<MasterDetector>(_detector, [](MasterDetector*) {});
```
But I don't think this is worth the extra complexity. (We would need to copy
the shared_ptr into the new no-op destructor and make sure that all future
tests understand this.)
> On Feb. 19, 2016, 3:46 p.m., Alexander Rojas wrote:
> > src/tests/cluster.cpp, lines 250-251
> > <https://reviews.apache.org/r/43613/diff/5/?file=1263029#file1263029line250>
> >
> > If we stick to `std::shared_ptr` I would suggest to change its
> > contstruction to `std::make_shared`
I'll also change the 4 tests that pass in this shared_ptr.
> On Feb. 19, 2016, 3:46 p.m., Alexander Rojas wrote:
> > src/tests/cluster.hpp, line 96
> > <https://reviews.apache.org/r/43613/diff/5/?file=1263028#file1263028line96>
> >
> > 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.
I was pondering this too.
I kept the "start" because this factory method calls `process::spawn` before
returning.
> On Feb. 19, 2016, 3:46 p.m., Alexander Rojas wrote:
> > src/tests/cluster.cpp, line 519
> > <https://reviews.apache.org/r/43613/diff/5/?file=1263029#file1263029line519>
> >
> > 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.
A gtest assert doesn't throw, it actually returns; which I think still makes
sense inside a destructor.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review119989
-----------------------------------------------------------
On Feb. 19, 2016, 11:36 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2016, 11:36 a.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
>
>
