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




src/Makefile.am (line 687)
<https://reviews.apache.org/r/45360/#comment193004>

    Please align the tailing ``



src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp (line 41)
<https://reviews.apache.org/r/45360/#comment193079>

    Do you want to use `const Option<string>& dvdcliPath` here? The idea is 
that if not specified, the client will just use 'dvdcli' in $PATH. You want to 
use os::which to do a check in 'create' to make sure it's there if dvdcliPath 
is not specified.



src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp (line 43)
<https://reviews.apache.org/r/45360/#comment193044>

    Are you using virtual because of testing?
    
    If not, let's try not to use virtual for now. Ditto for 'mount' and 
'unmount'. Let's just use concrete class with concrete implementation for now.
    
    If it's for testing, then i think that makse sense as you want to use a 
MockDockerVolumeDriverClient.



src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp (line 51)
<https://reviews.apache.org/r/45360/#comment193006>

    Is the idea here that we use 'None' to express the intention that we don't 
want to call 'Create' implicitly? If yes, please document this in the comments.
    
    Also, please remove 'const' for now as it's not clear it'll be const or not.



src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp (line 58)
<https://reviews.apache.org/r/45360/#comment193047>

    Ditto on removing 'const'.



src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp (line 60)
<https://reviews.apache.org/r/45360/#comment193048>

    Let's make it 'private' for now.



src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp (line 68)
<https://reviews.apache.org/r/45360/#comment193082>

    Why 'static'? If it's static, can that be a file local helper in driver.cpp?



src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp (lines 72 - 75)
<https://reviews.apache.org/r/45360/#comment193085>

    Ditto.



src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp (lines 58 - 63)
<https://reviews.apache.org/r/45360/#comment193102>

    Can you use the argv version of 'subprocess' to avoid the need for escaping 
special chars.
    ```
    vector<string> argv = {
      dvdcli,
      "mount",
      "--volumedriver=" + driver,
      "--volumename=" + name,
    };
    
    ...
    ```



src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp (lines 83 - 87)
<https://reviews.apache.org/r/45360/#comment193115>

    You need to call io::read() before the process terminates. Otherwise, if 
the pipe is full, the child process will block.
    
    ```
    return await(
        s->status(),
        s->out().get(),
        s->err().get())
      .then([](const tuple<
          Future<Option<int>>,
          Future<string>,
          Future<string>>& t) -> Future<string> {
        Future<Option<int>> status = std::get<0>(t);
        if (!status.isReady()) {
          ...
        }
        ...
      });
    ```
    
    Please follow the similar logic here:
    
https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/network/cni/cni.cpp#L807


- Jie Yu


On April 15, 2016, 2:43 p.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> -----------------------------------------------------------
> 
> (Updated April 15, 2016, 2:43 p.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added volume driver client for mount and unmount.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 06f58c4a88e3c527df727df1efe11ed3ab77efa8 
>   src/Makefile.am ec855263d620e4723c8ba9cd056c40a3a2e9ca99 
>   src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45360/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Reply via email to