mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
Date Tue, 01 May 2018 00:20:18 GMT

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


Fix it, then Ship it!




Overall LGTM. Thanks for your hard work!


src/tests/persistent_volume_tests.cpp
Lines 451 (patched)
<https://reviews.apache.org/r/66220/#comment283810>

    `that a framework can ... and receive the grown volume`



src/tests/persistent_volume_tests.cpp
Lines 459 (patched)
<https://reviews.apache.org/r/66220/#comment283799>

    Suggestion for the TODO: Make `MOUNT` a meaningful parameter value for this test, or create
a new fixture to avoid testing against it.
    
    BTW, would it be hard to do what you did for `MOUNT` in the `ShrinkVolume` test?



src/tests/persistent_volume_tests.cpp
Lines 500 (patched)
<https://reviews.apache.org/r/66220/#comment283800>

    `The disk spaces ... if the fixture parameter`



src/tests/persistent_volume_tests.cpp
Lines 505 (patched)
<https://reviews.apache.org/r/66220/#comment283801>

    `which does not`



src/tests/persistent_volume_tests.cpp
Lines 524 (patched)
<https://reviews.apache.org/r/66220/#comment283814>

    `containing the original volume`



src/tests/persistent_volume_tests.cpp
Lines 539 (patched)
<https://reviews.apache.org/r/66220/#comment283819>

    Could you leave the following comment here?
    ```
    // Ensure that the allocator has updated its resources before the next allocation.
    ```



src/tests/persistent_volume_tests.cpp
Lines 555 (patched)
<https://reviews.apache.org/r/66220/#comment283803>

    `Make sure the volume exists`



src/tests/persistent_volume_tests.cpp
Lines 565 (patched)
<https://reviews.apache.org/r/66220/#comment283802>

    `containing the grown volume`



src/tests/persistent_volume_tests.cpp
Lines 569 (patched)
<https://reviews.apache.org/r/66220/#comment283804>

    ```
    // Grow the volume.
    ```



src/tests/persistent_volume_tests.cpp
Lines 569 (patched)
<https://reviews.apache.org/r/66220/#comment283807>

    `Grow the volume.`



src/tests/persistent_volume_tests.cpp
Lines 575 (patched)
<https://reviews.apache.org/r/66220/#comment283805>

    Could you leave the following comment here?
    ```
    // Ensure that the allocator has updated its resources before the next allocation.
    ```



src/tests/persistent_volume_tests.cpp
Lines 575 (patched)
<https://reviews.apache.org/r/66220/#comment283806>

    Could you leave the following comment here?
    ```
    // Ensure that the allocator has updated its resources before the next allocation.
    ```



src/tests/persistent_volume_tests.cpp
Lines 601 (patched)
<https://reviews.apache.org/r/66220/#comment283811>

    `that a framework can ... see the shrunk volume`



src/tests/persistent_volume_tests.cpp
Lines 642 (patched)
<https://reviews.apache.org/r/66220/#comment283812>

    `The disk spaces ... if the fixture parameter`



src/tests/persistent_volume_tests.cpp
Lines 666 (patched)
<https://reviews.apache.org/r/66220/#comment283813>

    `containing the original volume`



src/tests/persistent_volume_tests.cpp
Lines 681 (patched)
<https://reviews.apache.org/r/66220/#comment283808>

    Could you leave the following comment here?
    ```
    // Ensure that the allocator has updated its resources before the next allocation.
    ```



src/tests/persistent_volume_tests.cpp
Lines 697 (patched)
<https://reviews.apache.org/r/66220/#comment283815>

    `Make sure the volume exists`



src/tests/persistent_volume_tests.cpp
Lines 707 (patched)
<https://reviews.apache.org/r/66220/#comment283816>

    `containing the shrunk volume`



src/tests/persistent_volume_tests.cpp
Lines 716 (patched)
<https://reviews.apache.org/r/66220/#comment283817>

    Could you leave the following comment here?
    ```
    // Ensure that the allocator has updated its resources before the next allocation.
    ```



src/tests/persistent_volume_tests.cpp
Lines 752 (patched)
<https://reviews.apache.org/r/66220/#comment283825>

    Remove "by".
    Suggestion: that a subsequent `LAUNCH` depending on a grown volume will be dropped



src/tests/persistent_volume_tests.cpp
Lines 754 (patched)
<https://reviews.apache.org/r/66220/#comment283824>

    Suggestion: How about `NonSpeculativeGrowAndLaunch`?



src/tests/persistent_volume_tests.cpp
Lines 760 (patched)
<https://reviews.apache.org/r/66220/#comment283826>

    Suggestion for the TODO: Make `MOUNT` a meaningful parameter value for this test, or create
a new fixture to avoid testing against it.



src/tests/persistent_volume_tests.cpp
Lines 801 (patched)
<https://reviews.apache.org/r/66220/#comment283827>

    `The disk spaces ... if the fixture parameter`



src/tests/persistent_volume_tests.cpp
Lines 806 (patched)
<https://reviews.apache.org/r/66220/#comment283828>

    `which does not`



src/tests/persistent_volume_tests.cpp
Lines 825 (patched)
<https://reviews.apache.org/r/66220/#comment283829>

    `containing the original volume`



src/tests/persistent_volume_tests.cpp
Lines 837 (patched)
<https://reviews.apache.org/r/66220/#comment283832>

    To make this test more succinct, how about doing
    `{CREATE(volume), GROW_VOLUME(volume, addition), LAUNCH(task)}` so we don't need the subsequent
`acceptOffers`?



src/tests/persistent_volume_tests.cpp
Lines 840 (patched)
<https://reviews.apache.org/r/66220/#comment283830>

    Could you leave the following comment here?
    ```
    // Ensure that the allocator has updated its resources before the next allocation.
    ```



src/tests/persistent_volume_tests.cpp
Lines 863 (patched)
<https://reviews.apache.org/r/66220/#comment283834>

    `containing the grown volume`



src/tests/persistent_volume_tests.cpp
Lines 867 (patched)
<https://reviews.apache.org/r/66220/#comment283835>

    `The volume will be grown but the task`



src/tests/persistent_volume_tests.cpp
Lines 877 (patched)
<https://reviews.apache.org/r/66220/#comment283833>

    If you decide to keep two `acceptOffers`, could you leave the following comment here?
    ```
    // Ensure that the allocator has updated its resources before the next allocation.
    ```



src/tests/persistent_volume_tests.cpp
Lines 896 (patched)
<https://reviews.apache.org/r/66220/#comment283836>

    Remove "by".
    Suggestion: that a subsequent `LAUNCH` depending on a shrunk volume will be dropped



src/tests/persistent_volume_tests.cpp
Lines 904 (patched)
<https://reviews.apache.org/r/66220/#comment283837>

    Suggestion for the TODO: Make `MOUNT` a meaningful parameter value for this test, or create
a new fixture to avoid testing against it.



src/tests/persistent_volume_tests.cpp
Lines 909 (patched)
<https://reviews.apache.org/r/66220/#comment283838>

    One line apart.



src/tests/persistent_volume_tests.cpp
Lines 962 (patched)
<https://reviews.apache.org/r/66220/#comment283839>

    `containing the original volume`



src/tests/persistent_volume_tests.cpp
Lines 974 (patched)
<https://reviews.apache.org/r/66220/#comment283843>

    To make this test more succinct, how about doing
    `{CREATE(volume), SHRINK_VOLUME(volume, subtract), LAUNCH(task)}` so we don't need the
subsequent `acceptOffers`?



src/tests/persistent_volume_tests.cpp
Lines 977 (patched)
<https://reviews.apache.org/r/66220/#comment283842>

    Could you leave the following comment here?
    ```
    // Ensure that the allocator has updated its resources before the next allocation.
    ```



src/tests/persistent_volume_tests.cpp
Lines 990 (patched)
<https://reviews.apache.org/r/66220/#comment283840>

    `containing the shrunk volume`



src/tests/persistent_volume_tests.cpp
Lines 1008 (patched)
<https://reviews.apache.org/r/66220/#comment283844>

    If you decide to keep two `acceptOffers`, could you leave the following comment here?
    ```
    // Ensure that the allocator has updated its resources before the next allocation.
    ```



src/tests/persistent_volume_tests.cpp
Lines 1015 (patched)
<https://reviews.apache.org/r/66220/#comment283841>

    `Expect an offer containing the shrunk volume`


- Chun-Hung Hsiao


On April 27, 2018, 8:04 p.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> -----------------------------------------------------------
> 
> (Updated April 27, 2018, 8:04 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
>     https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -----
> 
>   src/tests/persistent_volume_tests.cpp 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/8/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message