You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@apache.org> on 2018/05/01 00:20:18 UTC
Re: Review Request 66220: Added tests for `GROW_VOLUME` and
`SHRINK_VOLUME` operations.
-----------------------------------------------------------
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
>
>
Re: Review Request 66220: Added tests for `GROW_VOLUME` and
`SHRINK_VOLUME` operations.
Posted by Zhitao Li <zh...@gmail.com>.
> On April 30, 2018, 5:20 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 459 (patched)
> > <https://reviews.apache.org/r/66220/diff/8/?file=2014377#file2014377line459>
> >
> > 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?
Yes it is harder here because there is not conceivable way for a framework to receive both a `MOUNT` volume and free disk resource on the same `MOUNT`, so framework cannot even construct such operation from offered resources.
> On April 30, 2018, 5:20 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 837 (patched)
> > <https://reviews.apache.org/r/66220/diff/8/?file=2014377#file2014377line837>
> >
> > 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`?
I like this suggestion. will try it out.
- Zhitao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66220/#review202157
-----------------------------------------------------------
On April 27, 2018, 1: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, 1: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
>
>