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




src/tests/containerizer/docker_volume_isolator_tests.cpp (line 112)
<https://reviews.apache.org/r/46140/#comment195447>

    `sandbox->c_str()`
    
    Can you also log an ERROR `LOG(ERROR)` if unmountAll returns an Error?



src/tests/containerizer/docker_volume_isolator_tests.cpp (lines 132 - 133)
<https://reviews.apache.org/r/46140/#comment195448>

    This format looks weird to me.
    
    I think you can save a temporary 'Volume::Source' above:
    ```
    Volume volume
    volume.set_mode(Volume::RW);
    volume.set_container_path(...);
    
    Volume::Source* source = volume.mutable_source();
    source->set_type(Volume::Source::DOCKER_VOLUME);
    
    Volume::Source::DockerVolume* docker = source->mutable_docker_volume();
    docker->set_driver(driver);
    docker->set_name(name);
    
    if (parameters.isSome()) {
      ...
    }
    ```
    
    ALso, i think we should make 'parameters' of this helper to be a hashmap. 
It's easier for the callsite.



src/tests/containerizer/docker_volume_isolator_tests.cpp (line 143)
<https://reviews.apache.org/r/46140/#comment195449>

    Can you pass in an Owned<DriverClient>& instead?



src/tests/containerizer/docker_volume_isolator_tests.cpp (line 164)
<https://reviews.apache.org/r/46140/#comment195450>

    Kill this line.



src/tests/containerizer/docker_volume_isolator_tests.cpp (line 204)
<https://reviews.apache.org/r/46140/#comment195453>

    
`s/ROOT_LaunchCommandExecutorNoRootfsWithSingleVolume/ROOT_CommandTaskNoRootfs/`



src/tests/containerizer/docker_volume_isolator_tests.cpp (lines 214 - 227)
<https://reviews.apache.org/r/46140/#comment195452>

    I think you should move this down right before calling `launchTasks`.
    
    Also, please capture the parameter to the 'mount' and 'unmount' here to 
verify that it's the same as specified in ContainerInfo.



src/tests/containerizer/docker_volume_isolator_tests.cpp (lines 214 - 215)
<https://reviews.apache.org/r/46140/#comment195455>

    Is it used?



src/tests/containerizer/docker_volume_isolator_tests.cpp (line 223)
<https://reviews.apache.org/r/46140/#comment195454>

    is it used?



src/tests/containerizer/docker_volume_isolator_tests.cpp (line 267)
<https://reviews.apache.org/r/46140/#comment195451>

    What do you test here? I think you should pre-create some file in that 
volume and do `test -f ...` in the command.


- Jie Yu


On May 3, 2016, 5:12 a.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46140/
> -----------------------------------------------------------
> 
> (Updated May 3, 2016, 5:12 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-5266
>     https://issues.apache.org/jira/browse/MESOS-5266
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added test "ROOT_LaunchCommandExecutorNoRootfsWithSingleVolume".
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 53de98f43629dc94f7619324369caf88407b2f41 
>   src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp 
> 070d52018e82ed3e46fb1b79714ffc4716f6a306 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46140/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Reply via email to