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