You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Marco Massenzio <ma...@mesosphere.io> on 2015/08/11 17:57:06 UTC
Re: Review Request 36429: Add filesystem/linux isolator for persistent
volumes.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36429/#review94920
-----------------------------------------------------------
Wouldn't it be good if we could have some comments as to how this class is supposed to be used, what does it encapsulate, etc.?
At the very least a URL to a design doc or something?
Also, all the methods are completely undocumented: this means that, whenever anyone will want in future to use it, they will have to go hunt for the cpp file and reverse engineer the code (with a large amount of guessing as to the intent for the ambiguous parts) trying to figure out what each of them does.
(not to mention that it'll be anyone's guess what the methods' arguments are supposed to be).
I'm sure that for those 2-3 people who have spent the last year or so thinking about FSIsolators this is all obvious; but not so for those who haven't, and even less so for those poor souls who may want to join the project in future...
- Marco Massenzio
On July 12, 2015, 4:46 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36429/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:46 a.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Moved filesystem/linux from review https://reviews.apache.org/r/34135/
>
>
> Diffs
> -----
>
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION
> src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
>
> Diff: https://reviews.apache.org/r/36429/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 36429: Add filesystem/linux isolator for persistent
volumes.
Posted by Jie Yu <yu...@gmail.com>.
> On Aug. 11, 2015, 3:57 p.m., Marco Massenzio wrote:
> > Wouldn't it be good if we could have some comments as to how this class is supposed to be used, what does it encapsulate, etc.?
> >
> > At the very least a URL to a design doc or something?
> >
> > Also, all the methods are completely undocumented: this means that, whenever anyone will want in future to use it, they will have to go hunt for the cpp file and reverse engineer the code (with a large amount of guessing as to the intent for the ambiguous parts) trying to figure out what each of them does.
> >
> > (not to mention that it'll be anyone's guess what the methods' arguments are supposed to be).
> >
> > I'm sure that for those 2-3 people who have spent the last year or so thinking about FSIsolators this is all obvious; but not so for those who haven't, and even less so for those poor souls who may want to join the project in future...
FYI, this patch is being moved to as Ian is on vacation.
https://reviews.apache.org/r/37236/
https://reviews.apache.org/r/37330/
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36429/#review94920
-----------------------------------------------------------
On July 12, 2015, 4:46 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36429/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:46 a.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Moved filesystem/linux from review https://reviews.apache.org/r/34135/
>
>
> Diffs
> -----
>
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION
> src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
>
> Diff: https://reviews.apache.org/r/36429/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>