> On Feb. 26, 2016, 4:19 a.m., Bernd Mathiske wrote: > > src/tests/cluster.hpp, line 260 > > <https://reviews.apache.org/r/43613/diff/6/?file=1263429#file1263429line260> > > > > All this is nice to know, but ho does it connect to the variable > > "isShutdown" and why is it named this way? What does "isShutdown == true" > > signify? This should be in the comment IMHO. Or maybe a better variableName > > could make this more easy to grasp? Suggestion: > > > > bool cleanUpContainersInDestructor = true
I've reworked this group of associated comments; and renamed the bool while I was at it. I think its clearer now (and more verbose about why it exists). > On Feb. 26, 2016, 4:19 a.m., Bernd Mathiske wrote: > > src/tests/cluster.hpp, line 264 > > <https://reviews.apache.org/r/43613/diff/6/?file=1263429#file1263429line264> > > > > This global var makes the code hard to follow. > > > > And I am quite confused by the source code comment. > > > > Which is the case here? > > > > A. The tests in question execute shutdown logic themselves. So they > > still need the containerizer to be destroyed. But that's what isShutdown > > switches on and off futher down below, not the shutdown logic. > > > > B. The tests in question want the shutdown logic to be executed by > > "terminate()" but not the containerizer destruction. This is what setting > > isShutdown to true seems to cause. But the tests where I see terminate() in > > use sometimes need the containerizer destroyed, because they create a new > > one right away. And sometimes they do not. > > > > What am I missing? A) The tests execute shutdown logic themselves. They want to destroy the containerizer, **but not the containers**. `isShutdown` only determines whether we destroy containers. The containerizer's destructor does not destroy containers. B) Tests will do `slave->terminate()` to shutdown the slave. They need to do `slave.reset()` to call destructors on all the slave's dependencies. Tests need to recreate the containerizer because you cannot use the same containerizer instance over two slaves (i.e. you can't `containerizer->recover` twice). Tests that don't recreate the containerizer all use the `TestContainerizer`, which is just a mock anyway. (I'll leave this issue open so that you can close, if the new comments + my notes are sufficient.) > On Feb. 26, 2016, 4:19 a.m., Bernd Mathiske wrote: > > src/tests/cluster.cpp, line 504 > > <https://reviews.apache.org/r/43613/diff/6/?file=1263430#file1263430line504> > > > > Should this be placed after the cointainerizer destruction maybe? Say > > line 399. > > > > But then I still do not get to observe a consistent use pattern of > > terminate(). See my comment above. As noted above, the container**izer** isn't destructed here. The *containers* are. - Joseph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43613/#review120685 ----------------------------------------------------------- On Feb. 26, 2016, 11:50 a.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43613/ > ----------------------------------------------------------- > > (Updated Feb. 26, 2016, 11:50 a.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 > >
