----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44946/#review127166 -----------------------------------------------------------
Fix it, then Ship it! Looking good. Just a few minor comments, plus one mentioned in [another review](https://reviews.apache.org/r/44948/diff/11/?file=1321656#file1321656line140) and we are good to go. src/slave/containerizer/mesos/isolators/xfs/utils.hpp (lines 61 - 62) <https://reviews.apache.org/r/44946/#comment190394> s/directory/path/, this breaks the symmetry in the `*projectId` methods but this doesn't need to be a directory and we actually use it to test files in tests. src/slave/containerizer/mesos/isolators/xfs/utils.hpp (line 72) <https://reviews.apache.org/r/44946/#comment190312> Kill line. src/slave/containerizer/mesos/isolators/xfs/utils.hpp (line 78) <https://reviews.apache.org/r/44946/#comment190395> `s/__XFS_HPP__/__XFS_UTILS_HPP__`. src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 59) <https://reviews.apache.org/r/44946/#comment190321> Add an empty line. src/slave/containerizer/mesos/isolators/xfs/utils.cpp (lines 154 - 155) <https://reviews.apache.org/r/44946/#comment190404> I didn't mention this earlier but it would be nice if all these internal arithmetics use the Bytes object and only convert to a uint when it calls underlying systems calls. A TODO would be fine. src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 265) <https://reviews.apache.org/r/44946/#comment190398> "larger than or equal to" or ">=" so it matches the condition check. src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 266) <https://reviews.apache.org/r/44946/#comment190406> Nit: `stringify(Bytes(BASIC_BLOCK_SIZE))`. I have another comment about a TODO for using Bytes consistently, including the type of BASIC_BLOCK_SIZE. But OK to not do it in this review. src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 286) <https://reviews.apache.org/r/44946/#comment190396> `s/directory/path`. src/tests/environment.cpp (lines 468 - 471) <https://reviews.apache.org/r/44946/#comment190387> This is unrelated? - Jiang Yan Xu On April 4, 2016, 10:27 a.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44946/ > ----------------------------------------------------------- > > (Updated April 4, 2016, 10:27 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 f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 > src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION > src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION > src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 > > Diff: https://reviews.apache.org/r/44946/diff/ > > > Testing > ------- > > Make check. Manual verification. Tests in subsequent patches. > > > Thanks, > > James Peach > >
