-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44947/#review124445
-----------------------------------------------------------



Minor comments. More thorough review once the tests reflect the proposed new 
APIs.


src/tests/containerizer/xfs_quota_tests.cpp (line 17)
<https://reviews.apache.org/r/44947/#comment188324>

    Insert a blank line below.



src/tests/containerizer/xfs_quota_tests.cpp (line 51)
<https://reviews.apache.org/r/44947/#comment188325>

    `mesos:internal::xfs` goes alphabetically before `process` and leave a 
blank line in between.



src/tests/containerizer/xfs_quota_tests.cpp (line 102)
<https://reviews.apache.org/r/44947/#comment188410>

    End the sentence with '.'.



src/tests/containerizer/xfs_quota_tests.cpp (line 115)
<https://reviews.apache.org/r/44947/#comment188414>

    In the Mesos codebase C++ style argument comment is preferred: `0, // 
Flags.`



src/tests/containerizer/xfs_quota_tests.cpp (lines 133 - 135)
<https://reviews.apache.org/r/44947/#comment188547>

    The indentation here treats `subprocess(` as if it was a method argument.
    
    Suggestion:
    
    ```
    Try<Subprocess> s = subprocess(
        "losetup -d " + loopDevice.get(),
        Subprocess::PATH("/dev/null"));
    ```



src/tests/containerizer/xfs_quota_tests.cpp (line 138)
<https://reviews.apache.org/r/44947/#comment188423>

    We should give the wait a timeout. e.g., Seconds(15) as we don't want the 
tests to block forever under any circumstances.



src/tests/containerizer/xfs_quota_tests.cpp (line 150)
<https://reviews.apache.org/r/44947/#comment188539>

    s/a/an/



src/tests/containerizer/xfs_quota_tests.cpp (line 167)
<https://reviews.apache.org/r/44947/#comment188540>

    Doesn't look like we need `ftruncate` if we use `posix_fallocate`?
    
    And you are not checking the exit status here.
    
    IIUC, `os::ftruncate()` is not going to trigger ENOSPC but 
`::posix_fallocate` is. However the method is going to return `Nothing()`.



src/tests/containerizer/xfs_quota_tests.cpp (line 180)
<https://reviews.apache.org/r/44947/#comment188544>

    s/ctlfd/fd/



src/tests/containerizer/xfs_quota_tests.cpp (line 189)
<https://reviews.apache.org/r/44947/#comment188412>

    s/r//



src/tests/environment.cpp (line 467)
<https://reviews.apache.org/r/44947/#comment188323>

    Looks like #if also works but it's more standard to use #ifdef 
    
    http://www.faqs.org/docs/Linux-HOWTO/GCC-HOWTO.html


- Jiang Yan Xu


On March 26, 2016, 10:05 a.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44947/
> -----------------------------------------------------------
> 
> (Updated March 26, 2016, 10:05 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 tests for XFS project quota utilities.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 7617e43587cb81104786d06f753f08565a6c2d0a 
> 
> Diff: https://reviews.apache.org/r/44947/diff/
> 
> 
> Testing
> -------
> 
> Make check. Manual testing. These tests.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Reply via email to