----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66220/#review201235 -----------------------------------------------------------
Please consider unreplied comments as "will do". src/tests/persistent_volume_tests.cpp Lines 453 (patched) <https://reviews.apache.org/r/66220/#comment282370> Is the goal of splitting the test to have more focused tests? Otherwise, just taking out the task launching part from the code seems resulting in minimum amount of code. The file check actually verifies the fix for r/66218, which I think is something pretty important (not losing data during a grow/shrink). Only checking path exists for the volume would not capture that behavior. src/tests/persistent_volume_tests.cpp Lines 455-459 (patched) <https://reviews.apache.org/r/66220/#comment282372> I think we discussed about not allowing this but I forgot add this to validation. I'll update parent patch to include that validation. If we will drop any `GROW`/`SHRINK` operation, then we can verify that `GROW` does not trigger any `ApplyOperationMessage` and framework will be offered original volume as is and keep this well tested. src/tests/persistent_volume_tests.cpp Lines 492 (patched) <https://reviews.apache.org/r/66220/#comment282373> I do not see a possibility. Let me try to drop it and see what happens. src/tests/persistent_volume_tests.cpp Lines 541-542 (patched) <https://reviews.apache.org/r/66220/#comment282374> Let's capture this message and test its content to make sure proper operation is forwarded. src/tests/persistent_volume_tests.cpp Lines 597-603 (patched) <https://reviews.apache.org/r/66220/#comment282376> This was necessary if the operation is implemented as non-speculative, because master/allocator only sends out `converted` resources after receiving the feedback from agent. If we don't ensure this is processed, the offer could be subject to race condition (I think). Now that we changed the course, this may not be necessary in 1.6, but we'd need them again once we get to MESOS-8791. Please let me know if you think keeping it is still a bad idea. src/tests/persistent_volume_tests.cpp Lines 677-678 (patched) <https://reviews.apache.org/r/66220/#comment282378> Resources::contains only matches a volume if their size perfectly matches, so I think it is sufficient. - Zhitao Li 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 > >
