----------------------------------------------------------- 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 > >
