mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From haosdent huang <haosd...@gmail.com>
Subject Re: Review Request 45360: Added dvd client for mount and unmount.
Date Tue, 05 Apr 2016 16:55:00 GMT

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




src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp (line 28)
<https://reviews.apache.org/r/45360/#comment190254>

    I think add this here looks a bit wired.
    ```
    find . -name '*.hpp'|xargs grep 'using namespace'
    ./3rdparty/libprocess/3rdparty/stout/include/stout/lambda.hpp:using namespace std::placeholders;
    ./3rdparty/libprocess/include/process/pid.hpp: *    using namespace process;
    ./src/slave/slave.hpp:using namespace process;
    ```
    
    How about move the class to `messo::internal::slave::dvd` namespace?



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp (line 42)
<https://reviews.apache.org/r/45360/#comment190255>

    I suggest to follow `doxgen-style-guide.md` to add comments to functions.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 56)
<https://reviews.apache.org/r/45360/#comment190262>

    s/how does dvdcli works/how dvdcli works/g.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 58)
<https://reviews.apache.org/r/45360/#comment190257>

    I think we could use `strings::format` here?



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 59)
<https://reviews.apache.org/r/45360/#comment190263>

    Do we need check `dvdcli` version or something first?



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 65)
<https://reviews.apache.org/r/45360/#comment190258>

    I think could make it more excatly as well. `s/Mount command/Execute Docker Volume Driver
mount command:`.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 74)
<https://reviews.apache.org/r/45360/#comment190264>

    I suggest use `return Failure("Failed to mount Docker Volume Driver:" +  + s.error())`



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 111)
<https://reviews.apache.org/r/45360/#comment190266>

    A bit confuse why we continue to launch after unmount.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 125)
<https://reviews.apache.org/r/45360/#comment190269>

    I think this msg isn't clear. Actually we failed to launch DVD, but this message looks
like we failed in some general methods like `os::shell`. Maybe we could make it more specific.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 131)
<https://reviews.apache.org/r/45360/#comment190268>

    Add a line above.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 146)
<https://reviews.apache.org/r/45360/#comment190271>

    I suggest to replace `const Owned<ExternalMount>& externalMount,` to `Owned<ExternalMount>&
externalMount,` which make it more matched.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 148)
<https://reviews.apache.org/r/45360/#comment190270>

    I think you want to log `externalMount` here. You could `jsonify(JSON::Protobuf(*externalMount))`
directly.


- haosdent huang


On April 2, 2016, 5:56 a.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> -----------------------------------------------------------
> 
> (Updated April 2, 2016, 5:56 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added dvd client for mount and unmount.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt ff225c0d51a32b03a1b5f2ba31718ec2305c7ced 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.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