----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45360/#review128931 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (line 57) <https://reviews.apache.org/r/45360/#comment192375> How about using: `const stirng&` src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (line 58) <https://reviews.apache.org/r/45360/#comment192376> ditto. src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (lines 64 - 81) <https://reviews.apache.org/r/45360/#comment192379> It is still the issue I mentioned above: Since we have to specify `--volumedriver` and `--volumename`, and possibly have a bunch of `--volumeopt` appended. I would suggest collecting all this arguments (e.g., {dvdcliPath, "mount", "--volumedriver=XXXX", ...}) into a `vector<string> argv;`. and in subprocess, if you grep the examples `Try<subprocess>`, your can see for most cases we have cmd and argv separated. Considering the cmd may not be short. Let's make it more clear: ``` Try<Subprocess> s = subprocess( dvdcliPath, argv, Subprocess::PATH("/dev/null"), Subprocess::PIPE(), Subprocess::PIPE()); ``` src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (line 87) <https://reviews.apache.org/r/45360/#comment192385> Let's following the pattern in CNI isolator by using await here and passing the `tuple` to `launch()` instead of passing a subprocess. src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (line 104) <https://reviews.apache.org/r/45360/#comment192382> ditto. src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (lines 128 - 150) <https://reviews.apache.org/r/45360/#comment192389> Understand that you want to simplify the logic for mount and unmount here. Let us discuss this comparing the logic in CNI isolator `attach()` and `detach()` V.S. passing the reference of subprocess to launch. - Gilbert Song On April 13, 2016, 11:05 p.m., Guangya Liu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45360/ > ----------------------------------------------------------- > > (Updated April 13, 2016, 11:05 p.m.) > > > Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and > Jie Yu. > > > Repository: mesos > > > Description > ------- > > Added volume client for mount and unmount. > > > Diffs > ----- > > src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 > src/Makefile.am 139935fb40f8986427c85537a400ff945e54433f > src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp > PRE-CREATION > src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp > PRE-CREATION > > Diff: https://reviews.apache.org/r/45360/diff/ > > > Testing > ------- > > make > make check > > > Thanks, > > Guangya Liu > >