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


can you please set the dependency for this review correctly? it's hard to 
understand which reviews introduced the code that this review depends on.


include/mesos/slave/isolator.hpp (lines 70 - 71)
<https://reviews.apache.org/r/34137/#comment144221>

    We don't align arguments for constructors this way IIRC.
    
    ExecutorRunState(arg1,
                     arg2,
                     ....);



src/slave/containerizer/mesos/containerizer.cpp (line 400)
<https://reviews.apache.org/r/34137/#comment144231>

    kill new line.



src/slave/containerizer/mesos/containerizer.cpp (line 418)
<https://reviews.apache.org/r/34137/#comment144232>

    Is it possible for rootfs to not exist when we are here? If not, there 
should be a CHECK and a comment on what guarantees its presence (the fact that 
there is a forked pid?).



src/slave/containerizer/mesos/containerizer.cpp (lines 612 - 614)
<https://reviews.apache.org/r/34137/#comment144234>

    Why do you want to consider that? Don't we want to support both type of 
containers on the same host?



src/slave/containerizer/mesos/containerizer.cpp (lines 622 - 623)
<https://reviews.apache.org/r/34137/#comment144240>

    shouldn't there be a check to see if 'image' even exists in the protobuf?



src/slave/containerizer/mesos/containerizer.cpp (line 632)
<https://reviews.apache.org/r/34137/#comment144242>

    If you've extracted 'image' into a temp variable instead of 'type' above in 
#622, you could fit 'provision()' in one line.



src/slave/containerizer/mesos/containerizer.cpp (line 651)
<https://reviews.apache.org/r/34137/#comment144244>

    So what happens if the provisioner provisions the file system but the slave 
restarts before checkpointing 'rootfs'? Is that safe? We don't leak anything?



src/slave/containerizer/mesos/containerizer.cpp (line 790)
<https://reviews.apache.org/r/34137/#comment144245>

    when was flags.sandbox_directory introduced? which review?



src/slave/containerizer/mesos/containerizer.cpp (lines 815 - 816)
<https://reviews.apache.org/r/34137/#comment144247>

    +1



src/slave/containerizer/mesos/containerizer.cpp (line 817)
<https://reviews.apache.org/r/34137/#comment144251>

    where is the update to launchFlags to add 'rootfs' ? which review?



src/slave/containerizer/mesos/containerizer.cpp (line 1248)
<https://reviews.apache.org/r/34137/#comment144248>

    why include the containerid? doesn't the caller of this method already know 
that?



src/slave/containerizer/mesos/containerizer.cpp (line 1279)
<https://reviews.apache.org/r/34137/#comment144249>

    ditto. why include the container id?



src/slave/containerizer/provisioner.hpp (lines 59 - 60)
<https://reviews.apache.org/r/34137/#comment144228>

    What does this return?



src/slave/containerizer/provisioner.cpp (line 37)
<https://reviews.apache.org/r/34137/#comment144229>

    if (flags.provisioners.isNone()) {
    
    }



src/slave/flags.cpp (line 59)
<https://reviews.apache.org/r/34137/#comment144222>

    s/./(e.g., appc, docker)./


- Vinod Kone


On July 7, 2015, 7:42 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
> 
> (Updated July 7, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The MesosContainerizer can optionally provision a root filesystem for the 
> containers.
> 
> 
> Diffs
> -----
> 
>   include/mesos/slave/isolator.hpp 18edc030367e42240090f2f3dbc92aec7a4c6234 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 3ac2387eabded61c897a5016655aae10cd1bca91 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8c102fb7d1f79ee768cb06de3a976ea12f958712 
>   src/slave/containerizer/provisioner.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
>   src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f 
>   src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 
>   src/slave/state.hpp fed4b7ecf9572a8dbb1a99dbb1769d3e55ef7bc5 
>   src/slave/state.cpp 8eda22a550e5add0f84c46cc2ed762b006c0dcec 
>   src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e 
> 
> Diff: https://reviews.apache.org/r/34137/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Reply via email to