Re: Review Request 13502: First part of wrapper around the Isolator process

[Ben Mahler]

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


Thanks for taking this on Ian!


src/local/local.cpp
<https://reviews.apache.org/r/13502/#comment49410>

    s/IsolatorPtrs/IsolatorPointers/



src/local/local.cpp
<https://reviews.apache.org/r/13502/#comment49411>

    Hopefully we can remove the need for this map (see benh's TODO in 
shutdown()).
    
    For now can you inverse key / values?
    
    s/map<IsolatorPointers, Slave*>/map<Slave*, IsolatorPointers>/



src/local/local.cpp
<https://reviews.apache.org/r/13502/#comment49412>

    remove newline



src/local/local.cpp
<https://reviews.apache.org/r/13502/#comment49408>

    Not yours, but can you update this to be consistent with the deletion below?
    
    process::wait(master->self());
    
    delete master;
    master = NULL;
    
    delete allocator;
    allocator = NULL;
    
    delete allocatorProcess;
    allocatorProcess = NULL;



src/local/local.cpp
<https://reviews.apache.org/r/13502/#comment49420>

    s/isolator/isolatorPointers/



src/local/local.cpp
<https://reviews.apache.org/r/13502/#comment49426>

    Seems that we should flip the order of the pointers, so that we delete 
first followed by second:
    
    delete pointers.first;
    delete pointers.second;
    
    Can you apply the same change in the other bits where you used std::pair?



src/slave/isolator.hpp
<https://reviews.apache.org/r/13502/#comment49423>

    Can you declare Isolator above IsolatorProcess (e.g. files.hpp, gc.hpp), 
allocator.hpp doesn't do this but it should! ;)



src/slave/isolator.hpp
<https://reviews.apache.org/r/13502/#comment49342>

    s/ &/& /



src/slave/isolator.hpp
<https://reviews.apache.org/r/13502/#comment49343>

    additional newline



src/slave/isolator.hpp
<https://reviews.apache.org/r/13502/#comment49341>

    kill newline



src/slave/isolator.hpp
<https://reviews.apache.org/r/13502/#comment49424>

    kill virtuals



src/slave/isolator.hpp
<https://reviews.apache.org/r/13502/#comment49345>

    double newline here and between the other member function implementations 
below



src/slave/slave.cpp
<https://reviews.apache.org/r/13502/#comment49352>

    Can you kill this function and instead use a lambda::bind?
    
    You may find this helpful:
    
    
http://stackoverflow.com/questions/7999304/c-assigning-a-function-to-a-tr1function-object/7999412#7999412



src/tests/cluster.hpp
<https://reviews.apache.org/r/13502/#comment49414>

    Can you set these pointers to NULL after deletion, similar to what is done 
in local.cpp?



src/tests/environment.cpp
<https://reviews.apache.org/r/13502/#comment49427>

    CgroupsIsolatorProcess?



src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/13502/#comment49428>

    kill this :)



src/tests/mesos.cpp
<https://reviews.apache.org/r/13502/#comment49430>

    Ditto from above, deleting first, then second might be more intuitive?



src/tests/mesos.cpp
<https://reviews.apache.org/r/13502/#comment49418>

    s/isolatorPtrs/IsolatorPointers/


- Ben Mahler


On Aug. 12, 2013, 6:42 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13502/
> -----------------------------------------------------------
> 
> (Updated Aug. 12, 2013, 6:42 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-579
>     https://issues.apache.org/jira/browse/MESOS-579
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> First part of a wrapper around the Isolator process.
> 
> Does not yet return Futures instead of calling back to the slave.
> 
> 
> Diffs
> -----
> 
>   src/local/local.cpp e4b5ec5 
>   src/slave/cgroups_isolator.hpp e86062e 
>   src/slave/cgroups_isolator.cpp 3427c62 
>   src/slave/isolator.hpp fc13089 
>   src/slave/isolator.cpp c9643cf 
>   src/slave/main.cpp 750a127 
>   src/slave/monitor.cpp 4f3c91f 
>   src/slave/process_isolator.hpp 4ae093f 
>   src/slave/process_isolator.cpp a80b047 
>   src/slave/slave.hpp 8ba605b 
>   src/slave/slave.cpp 3b49118 
>   src/tests/cluster.hpp f743bb3 
>   src/tests/environment.cpp bf62bcf 
>   src/tests/fault_tolerance_tests.cpp c8d88d5 
>   src/tests/gc_tests.cpp e404de3 
>   src/tests/isolator.hpp fe6b38d 
>   src/tests/isolator_tests.cpp cd3b300 
>   src/tests/master_detector_tests.cpp 2d140ba 
>   src/tests/master_tests.cpp 52f09d4 
>   src/tests/mesos.hpp 8fbd56c 
>   src/tests/mesos.cpp 776cb0f 
>   src/tests/monitor_tests.cpp 3142416 
>   src/tests/slave_recovery_tests.cpp bd755f6 
> 
> Diff: https://reviews.apache.org/r/13502/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Ian Downes
> 
>