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