mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Guangya Liu <gyliu...@gmail.com>
Subject Re: Review Request 45360: Added dvd client for mount and unmount.
Date Wed, 13 Apr 2016 09:17:59 GMT


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


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