You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Zhitao Li <zh...@gmail.com> on 2018/04/11 21:19:46 UTC
Review Request 66569: Added a test to verify that grow and shrink
cannot be combined.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66569/
-----------------------------------------------------------
Review request for mesos, Chun-Hung Hsiao and Greg Mann.
Bugs: MESOS-4965
https://issues.apache.org/jira/browse/MESOS-4965
Repository: mesos
Description
-------
Added a test to verify that grow and shrink cannot be combined.
Diffs
-----
src/tests/persistent_volume_tests.cpp 4edf781711d9efdb994114aeb6289b6af750b87a
Diff: https://reviews.apache.org/r/66569/diff/1/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 66569: Added a test to verify that grow and shrink
cannot be combined.
Posted by Zhitao Li <zh...@gmail.com>.
> On April 16, 2018, 3:05 p.m., Chun-Hung Hsiao wrote:
> >
Thanks for the review. I'll combine this patch into r/66220 and address your comment in that process.
> On April 16, 2018, 3:05 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 691-695 (patched)
> > <https://reviews.apache.org/r/66569/diff/1/?file=1996763#file1996763line691>
> >
> > Since this test just verifies that the grow and shrink operations cannot be combined with other operations, I was wondering if there's a way to check this without worry about the different behaviors produced by PATH or MOUNT.
> >
> > Alternatively, if you take my suggestion to create another `PersistentVolumeResizeTest` fixture in the previous patch, we can move this test into that fixture.
Actually the check happens before type of `MOUNT` so this conditional check can be safely removed.
> On April 16, 2018, 3:05 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 775-781 (patched)
> > <https://reviews.apache.org/r/66569/diff/1/?file=1996763#file1996763line775>
> >
> > As discussed, I'm not sure if this is the semantics we want to enforce. If we end up rejecting the whole `ACCEPT` call, then we could simplify the test by:
> >
> > 1. Starting with a persistent volume, some free disk and some CPU.
> > 2. Open receiving an offer, accept it with reserving the CPU and growing the persistent volume. Different type of resources are used here to make sure it's not just for preventing pipelining.
> > 3. Verify that the `ACCEPT` call fails by checking that the next offer contains the original resources.
Semantic already fixed in the other patch. I will fix the test to reflect your suggested semantic.
- Zhitao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66569/#review201179
-----------------------------------------------------------
On April 11, 2018, 2:19 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66569/
> -----------------------------------------------------------
>
> (Updated April 11, 2018, 2:19 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test to verify that grow and shrink cannot be combined.
>
>
> Diffs
> -----
>
> src/tests/persistent_volume_tests.cpp 4edf781711d9efdb994114aeb6289b6af750b87a
>
>
> Diff: https://reviews.apache.org/r/66569/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 66569: Added a test to verify that grow and shrink
cannot be combined.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66569/#review201179
-----------------------------------------------------------
src/tests/persistent_volume_tests.cpp
Lines 691-695 (patched)
<https://reviews.apache.org/r/66569/#comment282236>
Since this test just verifies that the grow and shrink operations cannot be combined with other operations, I was wondering if there's a way to check this without worry about the different behaviors produced by PATH or MOUNT.
Alternatively, if you take my suggestion to create another `PersistentVolumeResizeTest` fixture in the previous patch, we can move this test into that fixture.
src/tests/persistent_volume_tests.cpp
Lines 702 (patched)
<https://reviews.apache.org/r/66569/#comment282405>
No need for this.
src/tests/persistent_volume_tests.cpp
Lines 728 (patched)
<https://reviews.apache.org/r/66569/#comment282407>
No need for this if the clock is already paused.
src/tests/persistent_volume_tests.cpp
Lines 732 (patched)
<https://reviews.apache.org/r/66569/#comment282227>
Let's move this to the beginning of this test to make it clear that this test pauses the clock.
src/tests/persistent_volume_tests.cpp
Lines 775-781 (patched)
<https://reviews.apache.org/r/66569/#comment282411>
As discussed, I'm not sure if this is the semantics we want to enforce. If we end up rejecting the whole `ACCEPT` call, then we could simplify the test by:
1. Starting with a persistent volume, some free disk and some CPU.
2. Open receiving an offer, accept it with reserving the CPU and growing the persistent volume. Different type of resources are used here to make sure it's not just for preventing pipelining.
3. Verify that the `ACCEPT` call fails by checking that the next offer contains the original resources.
src/tests/persistent_volume_tests.cpp
Lines 791 (patched)
<https://reviews.apache.org/r/66569/#comment282412>
No need for this.
src/tests/persistent_volume_tests.cpp
Lines 814-861 (patched)
<https://reviews.apache.org/r/66569/#comment282413>
How about moving the `SHRINK_VOLUME` test into another test? Let's keep the test focused.
- Chun-Hung Hsiao
On April 11, 2018, 9:19 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66569/
> -----------------------------------------------------------
>
> (Updated April 11, 2018, 9:19 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test to verify that grow and shrink cannot be combined.
>
>
> Diffs
> -----
>
> src/tests/persistent_volume_tests.cpp 4edf781711d9efdb994114aeb6289b6af750b87a
>
>
> Diff: https://reviews.apache.org/r/66569/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>