> On March 19, 2016, 10:10 p.m., Jie Yu wrote: > > src/linux/xfs.cpp, line 17 > > <https://reviews.apache.org/r/44946/diff/3/?file=1304831#file1304831line17> > > > > Move this after below stout headers. > > James Peach wrote: > The Mesos C++ style guide says that this should come first.
Sampled a few files, non of them are in this style. Either the style guide is not accurate, or we don't follow Mesos style guide consistently (which is bad). Fot the time being, let's just be consistent? > On March 19, 2016, 10:10 p.m., Jie Yu wrote: > > src/linux/xfs.cpp, line 53 > > <https://reviews.apache.org/r/44946/diff/3/?file=1304831#file1304831line53> > > > > Instead of relying on parameter, can we use os::stat::isdir here? > > James Peach wrote: > ``isdir`` always follows symlinks. From the caller of this we always make > sure what kind of inode we have so we don't need to ``stat(2)`` again and > potentially get it wrong. >From the API perspective, i just feel it strange that we need to pass around >'isdir' which we can always derive. Regarding the symlink semantics, we should clearly document the semantics for those public functions (i.e., do we follow symlink or not). I am wondering if we can do a symlink (`os::stat::islink`) check in those public functions. For instance, return Nothing if 'path' passed to clearProjectId is a symlink. > On March 19, 2016, 10:10 p.m., Jie Yu wrote: > > src/linux/xfs.cpp, line 187 > > <https://reviews.apache.org/r/44946/diff/3/?file=1304831#file1304831line187> > > > > This interface looks weird. I understand that project quota is set on > > the device level. Can we make it explicit? and expose another public method > > to get device by path? > > James Peach wrote: > This is not a general-purpose interface; it's just here for the XFS > isolator. If we require a device path here, then we just foist that work off > onto the caller. I think that it is simpler to hide the device requirement > from that level and just do it implicitly (at least until we can show there's > a performance win to allowing the caller to cache it). I made this comment due to the confusion I got while reviewing the code. My first reaction was that quota for a given project id is tied to a given 'path'. I am pretty sure other readers will have the same confusion until they read the code more. I just thougth we should make the API semantics more clear to improve readability. - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44946/#review124427 ----------------------------------------------------------- On March 21, 2016, 2 a.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44946/ > ----------------------------------------------------------- > > (Updated March 21, 2016, 2 a.m.) > > > Review request for mesos, Jie Yu and Jiang Yan Xu. > > > Bugs: MESOS-4828 > https://issues.apache.org/jira/browse/MESOS-4828 > > > Repository: mesos > > > Description > ------- > > Add utility functions to manipulate XFS project quotas. > > > Diffs > ----- > > src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf > src/linux/xfs.hpp PRE-CREATION > src/linux/xfs.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/44946/diff/ > > > Testing > ------- > > Make check. Manual verification. Tests in subsequent patches. > > > Thanks, > > James Peach > >
