> On 四月 5, 2016, 4:55 p.m., haosdent huang wrote: > > > > haosdent huang wrote: > By the way, I saw you didn't support `create`, `remove` and `path` > methods while it exists in dvdcli. Are they unnecessary here?
We do not need to use those methods but only `mount` and `umount`. 1) We expect the end user create the volume first before use it. 2) Similar as docker, it is not expected to remove volume by mesos, as the data in the volume may be needed in future, this should leave to end user. > On 四月 5, 2016, 4:55 p.m., haosdent huang wrote: > > src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp, line 59 > > <https://reviews.apache.org/r/45360/diff/2/?file=1323003#file1323003line59> > > > > Do we need check `dvdcli` version or something first? The dvdcli currently only have one release and also planning to write a C++ library for mesos, so I think that we do not need to check the version now. What do you think? https://github.com/emccode/dvdcli/releases > On 四月 5, 2016, 4:55 p.m., haosdent huang wrote: > > src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp, line 111 > > <https://reviews.apache.org/r/45360/diff/2/?file=1323003#file1323003line111> > > > > A bit confuse why we continue to launch after unmount. I need to check the unmount result in `launch`. > On 四月 5, 2016, 4:55 p.m., haosdent huang wrote: > > src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp, line 125 > > <https://reviews.apache.org/r/45360/diff/2/?file=1323003#file1323003line125> > > > > 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. I was following the code style here: https://github.com/apache/mesos/blob/master/src/docker/docker.cpp#L163 Can you please show more why do you think the message is not clear? Any proposal? ;-) > On 四月 5, 2016, 4:55 p.m., haosdent huang wrote: > > src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp, line 146 > > <https://reviews.apache.org/r/45360/diff/2/?file=1323003#file1323003line146> > > > > I suggest to replace `const Owned<ExternalMount>& externalMount,` to > > `Owned<ExternalMount>& externalMount,` which make it more matched. What is the problem of using `const` here? > On 四月 5, 2016, 4:55 p.m., haosdent huang wrote: > > src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp, line 58 > > <https://reviews.apache.org/r/45360/diff/2/?file=1323003#file1323003line58> > > > > I think we could use `strings::format` here? Using "+" may be more simple, the `strings::format` will return a `Try`. I saw that docker is using such way https://github.com/apache/mesos/blob/master/src/docker/docker.cpp#L142 , what do you think? - Guangya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45360/#review127121 ----------------------------------------------------------- On 四月 13, 2016, 9:17 a.m., Guangya Liu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45360/ > ----------------------------------------------------------- > > (Updated 四月 13, 2016, 9:17 a.m.) > > > Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and > Jie Yu. > > > Repository: mesos > > > Description > ------- > > Added dvd client for mount and unmount. > > > Diffs > ----- > > src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 > src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5 > 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 > >
