mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Zhitao Li <zhitaoli...@gmail.com>
Subject Re: Review Request 66220: Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
Date Thu, 19 Apr 2018 16:24:45 GMT


> On April 15, 2018, 5:39 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 453 (patched)
> > <https://reviews.apache.org/r/66220/diff/4/?file=1996762#file1996762line453>
> >
> >     If I'm not mistaken, this test does the following steps:
> >     
> >     1. Create a persistent volume
> >     2. Launch a task to write a file in the volume
> >     3. Grow the volume
> >     4. Check if the file exists in the volume
> >     5. Shrink the volume
> >     6. Check if the file exists in the volume
> >     
> >     The majority of this test is to implement Step 1 and 2, which are already testsed
in `AccessPersistentVolume` and not really relevent to the growing and shrinking functions.
How about splitting this test into two tests, `GrowPersistentVolume` and `ShrinkPersistentVolume`,
where the first one only does Step 1, 3, 4 and the second only does Step 1, 5, 6? The creation
of the file can be done with a simple `os::write()` to the volume path instead of using a
task.
> >     
> >     Also, I'm not sure if the file check is really necessary. If not, we can just
check the existence of the volume path in Step 4 and 6. Furthermore, we could even choose
not to check the existence of the volume path. In this case, we could make the persistent
volume in `slaveFlags.resources` from the very beginning, and completely get rid of Step 4
and 6, and thus the whole test would be much easier to read.

The file check is definitely necessary IMO, because we are guaranteed that content of volume
is not affected during grow/shrink, but I'm okay to use a file operation directly rather than
a task to perform the check.

+1 for splitting the test.

w.r.t to directly creating volume from the very beginning, isn't persistent volume controlled
by checkpointed info rather than `slaveFlags.resources`?


> On April 15, 2018, 5:39 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 519-570 (patched)
> > <https://reviews.apache.org/r/66220/diff/4/?file=1996762#file1996762line519>
> >
> >     I feel the whole task launch doesn't need to be tested in this unit test. See
my comments above.

Will convert to file operation directly.


- Zhitao


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


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/66220/
> -----------------------------------------------------------
> 
> (Updated April 11, 2018, 2:19 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 test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -----
> 
>   src/tests/persistent_volume_tests.cpp 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/4/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>


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