----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66220/#review201109 -----------------------------------------------------------
src/tests/persistent_volume_tests.cpp Lines 455-459 (patched) <https://reviews.apache.org/r/66220/#comment282073> Is this enforced somewhere in validation code? Can we check for expected behavior when a GROW/SHRINK operation is submitted for a MOUNT volume, rather than simply returning? src/tests/persistent_volume_tests.cpp Lines 471-479 (patched) <https://reviews.apache.org/r/66220/#comment282074> Nit: could you group all of the agent dependencies together? i.e. ``` Future<UpdateSlaveMessage> updateSlaveMessage = FUTURE_PROTOBUF(UpdateSlaveMessage(), _, _); slave::Flags slaveFlags = CreateSlaveFlags(); slaveFlags.resources = getSlaveResources(); Owned<MasterDetector> detector = master.get()->createDetector(); Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(), slaveFlags); ASSERT_SOME(slave); ``` src/tests/persistent_volume_tests.cpp Lines 492 (patched) <https://reviews.apache.org/r/66220/#comment282078> Is this necessary? Since the clock is paused, is it really possible that we'll get more offers than expected? src/tests/persistent_volume_tests.cpp Lines 496 (patched) <https://reviews.apache.org/r/66220/#comment282076> Could you move this to the top of the test to make it clearer that we will have the clock paused for the whole test? src/tests/persistent_volume_tests.cpp Lines 541-542 (patched) <https://reviews.apache.org/r/66220/#comment282080> Is this `Future` necessary? Since the task consumes the volume, it may be sufficient to await on the task status updates? src/tests/persistent_volume_tests.cpp Lines 542 (patched) <https://reviews.apache.org/r/66220/#comment282077> Indented too far. src/tests/persistent_volume_tests.cpp Lines 549 (patched) <https://reviews.apache.org/r/66220/#comment282079> Is this necessary? src/tests/persistent_volume_tests.cpp Lines 553-554 (patched) <https://reviews.apache.org/r/66220/#comment282081> Is this necessary? Awaiting on the task status updates may be sufficient? src/tests/persistent_volume_tests.cpp Lines 557 (patched) <https://reviews.apache.org/r/66220/#comment282082> Suggestion: you could use the `TaskStatusTaskIdEq` matcher in the `EXPECT_CALL(sched, statusUpdate(&driver, _))` call to avoid these expectations for the task ID. src/tests/persistent_volume_tests.cpp Lines 597-603 (patched) <https://reviews.apache.org/r/66220/#comment282084> I'm not sure if these expectations are necessary - perhaps it's sufficient to confirm that the subsequent offer contains a volume of the expected size? Ditto for the `shrinkVolumeMessage` below. src/tests/persistent_volume_tests.cpp Lines 613 (patched) <https://reviews.apache.org/r/66220/#comment282083> Is this comment correct? Won't this offer contain the grown volume? src/tests/persistent_volume_tests.cpp Lines 634-636 (patched) <https://reviews.apache.org/r/66220/#comment282087> I think these should be indented two more spaces. src/tests/persistent_volume_tests.cpp Lines 657-658 (patched) <https://reviews.apache.org/r/66220/#comment282088> Is this necessary? src/tests/persistent_volume_tests.cpp Lines 668 (patched) <https://reviews.apache.org/r/66220/#comment282091> Is this necessary? src/tests/persistent_volume_tests.cpp Lines 680 (patched) <https://reviews.apache.org/r/66220/#comment282075> Is this necessary? - Greg Mann 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/66220/ > ----------------------------------------------------------- > > (Updated April 11, 2018, 9: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 > >
