mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 45360: Added volume driver client for mount and unmount.
Date Tue, 19 Apr 2016 22:37:01 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message