----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52295/#review152420 -----------------------------------------------------------
src/tests/hierarchical_allocator_tests.cpp (lines 1512 - 1513) <https://reviews.apache.org/r/52295/#comment221467> The way this test is written, it's not testing special properties of shared persistent volumes at all. Persistent volumes have the same behavior... Shared persistent volumes are offered one-by-one as well but are "offerable even if it's already in use". We should test that instead. src/tests/hierarchical_allocator_tests.cpp (line 1519) <https://reviews.apache.org/r/52295/#comment221424> s/with opt/opted in/? src/tests/hierarchical_allocator_tests.cpp (lines 1530 - 1533) <https://reviews.apache.org/r/52295/#comment221468> Is 3 better than 2 in this case? Can we shorten this test? src/tests/hierarchical_allocator_tests.cpp (lines 1580 - 1581) <https://reviews.apache.org/r/52295/#comment225079> We don't use this style. Instead: ``` EXPECT_EQ( allocation.get().resources.get(slave.id()).get().shared(), Resources(volume)); ``` src/tests/hierarchical_allocator_tests.cpp (line 2158) <https://reviews.apache.org/r/52295/#comment221469> This test is basically identitcal to 'AllocateSharedResourcesMultiFrameworks', can we templatize it? src/tests/master_validation_tests.cpp (lines 674 - 681) <https://reviews.apache.org/r/52295/#comment221470> Use `createTask()`. src/tests/master_validation_tests.cpp (lines 683 - 690) <https://reviews.apache.org/r/52295/#comment221471> The existence of `task2` (and `disk2` for that matter) doesn't seem necessary? They use disctint resources and can't imagine them interfere with each other? Shorten the test? src/tests/persistent_volume_tests.cpp (lines 1002 - 1003) <https://reviews.apache.org/r/52295/#comment225115> Can we add how we verify "the master recovers"? e.g., ``` // This test verifies that the master recovers after a failover and re-offers the shared persistent volume when tasks using the same volume are still running. ``` src/tests/persistent_volume_tests.cpp (line 1004) <https://reviews.apache.org/r/52295/#comment221660> s/SharedVolume/SharedPersistentVolume/ Full spelling helps with code grepping. src/tests/persistent_volume_tests.cpp (lines 1006 - 1008) <https://reviews.apache.org/r/52295/#comment225098> No `masterFlags` necessary. src/tests/persistent_volume_tests.cpp (line 1011) <https://reviews.apache.org/r/52295/#comment221472> With the new test helper we now use: ``` Owned<MasterDetector> detector = master.get()->createDetector(); Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(), slaveFlags); ``` src/tests/persistent_volume_tests.cpp (lines 1050 - 1065) <https://reviews.apache.org/r/52295/#comment225109> Intead of actually writing files, we should probably keep them up via "sleep 1000" so they are running during the failover. BTW we need to have expectations for status updates otherwise there are going to be warnings? src/tests/persistent_volume_tests.cpp (lines 1089 - 1090) <https://reviews.apache.org/r/52295/#comment225112> Nit: The relevant slave behavior can be inferred from the offer so we don't need this expectation. src/tests/sorter_tests.cpp (line 637) <https://reviews.apache.org/r/52295/#comment225610> s/disk 50/disk of 50/ src/tests/sorter_tests.cpp (lines 660 - 661) <https://reviews.apache.org/r/52295/#comment225611> This actually fits one line? If not, the indentation should be ``` Resources additionalAShared = Resources(sharedDisk) + sharedDisk + sharedDisk; ``` src/tests/sorter_tests.cpp (line 705) <https://reviews.apache.org/r/52295/#comment225612> s/the fair/its dominant/ src/tests/sorter_tests.cpp (line 722) <https://reviews.apache.org/r/52295/#comment225614> For this test, I think a simple implementation would be to `add()` the same resources twice and `remove()` it twice. The `totalScalarQuantities()` only drops after the second `remove()`. src/tests/sorter_tests.cpp (lines 733 - 738) <https://reviews.apache.org/r/52295/#comment225613> Mesos wouldn't add the same shared resources in this way. Call `sorter->add()` multiple times instead? - Jiang Yan Xu On Sept. 26, 2016, 5:39 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52295/ > ----------------------------------------------------------- > > (Updated Sept. 26, 2016, 5:39 p.m.) > > > Review request for mesos and Jiang Yan Xu. > > > Bugs: MESOS-6185 > https://issues.apache.org/jira/browse/MESOS-6185 > > > Repository: mesos > > > Description > ------- > > Tests include the following: > - Allocator tests when dealing with multiple frameworks of the same > role using the same shared volume at the same time. > - Validity of DESTROY shared volume when there are pending tasks > that have been assigned that shared volume. > - Handlng of master failover. > - Sorter tests covering shared resources. > > > Diffs > ----- > > src/tests/hierarchical_allocator_tests.cpp > 46553f6d501deb37862dd5ba48be1c114f6e6cb8 > src/tests/master_validation_tests.cpp > 16c5773aa44016f923e00cb348ded6b8c46d4b4b > src/tests/persistent_volume_tests.cpp > 2ab44d11d159162dfcac9d0791b651ed059b8164 > src/tests/sorter_tests.cpp 1f17c011898836ea9159661dde7d544cb0d8ea83 > > Diff: https://reviews.apache.org/r/52295/diff/ > > > Testing > ------- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
