-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43613/#review120685
-----------------------------------------------------------




src/tests/cluster.hpp (line 111)
<https://reviews.apache.org/r/43613/#comment182144>

    s/were/are/



src/tests/cluster.hpp (line 181)
<https://reviews.apache.org/r/43613/#comment182368>

    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



src/tests/cluster.hpp (line 185)
<https://reviews.apache.org/r/43613/#comment182369>

    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?



src/tests/cluster.cpp (line 368)
<https://reviews.apache.org/r/43613/#comment182370>

    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.


- Bernd Mathiske


On Feb. 24, 2016, 12:06 p.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> -----------------------------------------------------------
> 
> (Updated Feb. 24, 2016, 12:06 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
> 
>

Reply via email to