This is an automatically generated e-mail. To reply, visit:

src/tests/containerizer/xfs_quota_tests.cpp (line 338)

    These more complex and integration oriented tests should all have a header 
comment so it makes it easier for 
    Something like:
    // This test verifies that a task fails to write to disk when it 
    // exceeds the requested disk quota.

src/tests/containerizer/xfs_quota_tests.cpp (line 345)

    Two spaces to continue after `=`.
    Here and elsewhere in this test.

src/tests/containerizer/xfs_quota_tests.cpp (line 357)

    One space between `;` and `//`.
    I saw some other test code that does this but it's probably due to copying 
and pasting of bad examples. Unless it's to align with some other comments, use 
one space.
    Here and elsewhere.

src/tests/containerizer/xfs_quota_tests.cpp (line 390)


src/tests/containerizer/xfs_quota_tests.cpp (line 425)

    `s/Future<vector<Offer> >/Future<vector<Offer>>/` here and elsewhere.

src/tests/containerizer/xfs_quota_tests.cpp (line 461)

    `Timeout::in()` and `Timeout::expired()` is more suitable for this.

src/tests/containerizer/xfs_quota_tests.cpp (line 486)

    With Timeout you don't need to do this.

src/tests/containerizer/xfs_quota_tests.cpp (lines 526 - 531)

    Here we seem to be implying that we are expecting the task to die because 
it exceeds the disk quota but in fact we are just testing slave recovery and 
the task doesn't die because of the `;`.
    How about we just request 1MB and use 1MB (or not use anything at all (just 
sleep) since we are mainly verifying if the project ID is cleared afterwards.

src/tests/containerizer/xfs_quota_tests.cpp (line 545)

    Two spaces.

src/tests/containerizer/xfs_quota_tests.cpp (line 551)

    This is not guaranteed if `dd` is slow to write to the disk and the rest of 
the test proceeds quickly.
    I think it's OK if we don't verify this here because the two tests above 
already verified that disk usage is correctly reported and correctly capped at 
the limit.

src/tests/containerizer/xfs_quota_tests.cpp (lines 565 - 566)

    This is not necessary if we
    because reregistration only happens after `Slave::_recover`.

src/tests/containerizer/xfs_quota_tests.cpp (line 590)

    Additionally we should assert there is exactly one container. Otherwise the 
foreach below can fall through without triggering any expectations if 
`sandboxees` is empty.

src/tests/containerizer/xfs_quota_tests.cpp (line 593)

    `s/foreach(/foreach (/`

src/tests/containerizer/xfs_quota_tests.cpp (lines 680 - 691)

    This part is not necessary. Slave considers itself fully recovered when the 
containers are fully recovered, not when the executors have all reregistered. 
Also the exchange of reconnect and reregister messages doesn't go through a 
delay so no need to advance (only the `reregisterExecutorTimeout` goes through 
a delay).
    Simply removing these lines should do it.

src/tests/containerizer/xfs_quota_tests.cpp (line 702)

    This is again not guaranteed. I think it's good enough if we just verify 
the limits.

src/tests/containerizer/xfs_quota_tests.cpp (line 718)

    `s/foreach(/foreach (/`

- Jiang Yan Xu

On April 6, 2016, 3:37 p.m., James Peach wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44949/
> -----------------------------------------------------------
> (Updated April 6, 2016, 3:37 p.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 basic XFS disk isolator tests by cloning the POSIX disk isolator
> tests and making minor changes for the differences in semantics.
> Diffs
> -----
>   src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
> Diff: https://reviews.apache.org/r/44949/diff/
> Testing
> -------
> Make check. Manual testing.
> Thanks,
> James Peach

Reply via email to