----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57964/#review172689 -----------------------------------------------------------
src/tests/persistent_volume_tests.cpp Lines 1174-1175 (patched) <https://reviews.apache.org/r/57964/#comment245767> The comment doesn't seem right? src/tests/persistent_volume_tests.cpp Lines 1176 (patched) <https://reviews.apache.org/r/57964/#comment245738> s/SharedVolume/SharedPersistentVolume/ for consistent with the other tests (for greppability). In fact, given ``` 912:TEST_P(PersistentVolumeTest, SharedPersistentVolumeMultipleTasks) 1022:TEST_P(PersistentVolumeTest, SharedPersistentVolumeRescindOnDestroy) 1177:TEST_P(PersistentVolumeTest, SharedPersistentVolumeMasterFailover) 1525:TEST_P(PersistentVolumeTest, SharedPersistentVolumeMultipleIterations) ``` Perhaps "SharedPersistentVolumeMultipleFrameworks" or "SharedPersistentVolumeMetricsMultipleFrameworks"? It looks like we didn't have a test that covers multiple frameworks launching tasks onto the same volume so I feel that "MultipleFrameworks" is worth mentioning. And frankly we are testing more than just the metrics: we are testing the two tasks can both write the the same volume, etc. So this is just a general "multiple tasks from multiple frameworks" test. We can make the intention of this test clear in the comment above. src/tests/persistent_volume_tests.cpp Lines 1211-1212 (patched) <https://reviews.apache.org/r/57964/#comment245741> I see `Clock::advance(slaveFlags.registration_backoff_factor);` is for the agent's initial backoff but the allocation should be immediate without clock advancement? src/tests/persistent_volume_tests.cpp Lines 1230-1232 (patched) <https://reviews.apache.org/r/57964/#comment245742> The test below does use the volume and depend on the volume being created and in this test I guess we have two legit tasks using the volume so perhaps just kill the last sentence? src/tests/persistent_volume_tests.cpp Lines 1255-1256 (patched) <https://reviews.apache.org/r/57964/#comment245768> Actually this doesn't look necessary, we already wait for the task status update below. src/tests/persistent_volume_tests.cpp Lines 1261 (patched) <https://reviews.apache.org/r/57964/#comment245744> s/framework 1/framework1/? src/tests/persistent_volume_tests.cpp Lines 1263-1266 (patched) <https://reviews.apache.org/r/57964/#comment245770> Nit: Strictly speaking I think all the `stats1.values.count` ones should be `ASSERT_EQ` (these should always be there and you should abort the test if they didn't) but I see that it's not done consistently. src/tests/persistent_volume_tests.cpp Lines 1298 (patched) <https://reviews.apache.org/r/57964/#comment245747> You did `EXPECT_FALSE(offers1->empty());` above, do the same here? src/tests/persistent_volume_tests.cpp Lines 1307 (patched) <https://reviews.apache.org/r/57964/#comment245748> "a portion of the offered resources" doesn't have any significance here so we can simplify the comment? Here what's worth pointing out is just "task2 uses the same shared volume as task1". src/tests/persistent_volume_tests.cpp Lines 1313 (patched) <https://reviews.apache.org/r/57964/#comment245764> "sleep 1000" above but "sleep 500" here? Keep them consistent ("sleep 1000")? src/tests/persistent_volume_tests.cpp Lines 1326-1327 (patched) <https://reviews.apache.org/r/57964/#comment245769> Actually this doesn't look necessary, we already wait for the task status update below. - Jiang Yan Xu On March 27, 2017, 3:27 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57964/ > ----------------------------------------------------------- > > (Updated March 27, 2017, 3:27 p.m.) > > > Review request for mesos and Jiang Yan Xu. > > > Bugs: MESOS-7186 > https://issues.apache.org/jira/browse/MESOS-7186 > > > Repository: mesos > > > Description > ------- > > Added a test to verify metrics when shared resources are present. > > > Diffs > ----- > > src/tests/persistent_volume_tests.cpp > 3eb7afe3de8e72ffb6502dfe12ef37ccd4ca2125 > > > Diff: https://reviews.apache.org/r/57964/diff/2/ > > > Testing > ------- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
