----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54449/#review167666 -----------------------------------------------------------
Fix it, then Ship it! Sorry this was mostly done but got overlooked due to holidays... We have discussed the unit test before and it's probably fine if we punt on that (I still think a negative test case is useful) but could you update the "Testing Done" section with a manual test negative case result? The negative case is the whole point of this patch after all. :) src/slave/containerizer/mesos/isolators/xfs/utils.cpp Lines 408-412 (patched) <https://reviews.apache.org/r/54449/#comment239583> This is very thorough, thanks. :) I originally just meant that we could comment on what each `0` means on their own line, much like (this)[https://github.com/apache/mesos/blob/cc15db4b10af343d7aefc838bb626f4579289375/src/tests/containerizer/xfs_quota_tests.cpp#L128] and perhaps copy some snippets from the external doc for the sake of "explaining obscure APIs/terms". Would it make sense to s/we are getting/it is for/? The difference to me is whether the API has to be used this way or we are using the API in this particular way. Sounds like the former is true? src/slave/containerizer/mesos/isolators/xfs/utils.cpp Lines 412 (patched) <https://reviews.apache.org/r/54449/#comment239584> s/identity/identity (e.g., projectId)/? In this file we have been using `projectId` throughout and I as a reader became more familiar with that term than `identity`. - Jiang Yan Xu On March 1, 2017, 10:50 a.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54449/ > ----------------------------------------------------------- > > (Updated March 1, 2017, 10:50 a.m.) > > > Review request for mesos, Jie Yu and Jiang Yan Xu. > > > Bugs: MESOS-6732 > https://issues.apache.org/jira/browse/MESOS-6732 > > > Repository: mesos > > > Description > ------- > > The XFS disk isolator checks that the filesystem is XFS, but doesn't > check whether project quotas are actually enabled. This means that > an invalid configuration will start but will always fail when tasks > are launched. > > Add a check to test whether project quotas are enabled on the work > directory and fail hard if they are not. > > > Diffs > ----- > > configure.ac 7a18c89854c1f42bec09f067579b72114fb8ec1d > src/slave/containerizer/mesos/isolators/xfs/disk.cpp > dd4df86bf90bfa9cbf4664d89274cf3c64c2e374 > src/slave/containerizer/mesos/isolators/xfs/utils.hpp > 7602fe3b6ab069db643397418732e773d0417f8a > src/slave/containerizer/mesos/isolators/xfs/utils.cpp > b9d8e7dc999ba3064bee7105eff0f9553d825df8 > > > Diff: https://reviews.apache.org/r/54449/diff/3/ > > > Testing > ------- > > Make check on Fedora 25. Manual test on F25 with mesos-execute. > > > Thanks, > > James Peach > >
