----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45964/#review146219 -----------------------------------------------------------
src/tests/hierarchical_allocator_tests.cpp (lines 1269 - 1270) <https://reviews.apache.org/r/45964/#comment213413> This test specifically tests CREATE/DESTROY for shared volumes so let's be very specific: ``` // This test verifies that `updateAllocation()` supports creating and destroying shared persistent volumes. ``` src/tests/hierarchical_allocator_tests.cpp (line 1271) <https://reviews.apache.org/r/45964/#comment212591> s/UpdateShareAllocation/UpdateAllocationSharedPersistentVolume/ src/tests/hierarchical_allocator_tests.cpp (line 1277) <https://reviews.apache.org/r/45964/#comment212588> Use `{}` below. src/tests/hierarchical_allocator_tests.cpp (line 1280) <https://reviews.apache.org/r/45964/#comment212589> i.e., use ``` allocator->addSlave(slave.id(), slave, None(), slave.resources(), {}); ``` src/tests/hierarchical_allocator_tests.cpp (lines 1305 - 1306) <https://reviews.apache.org/r/45964/#comment212690> s/update1/update/ src/tests/hierarchical_allocator_tests.cpp (lines 1349 - 1350) <https://reviews.apache.org/r/45964/#comment212695> You don't need `update2`, you *expect* `update2` to be `slave.resources`, so just use `slave.resources`. We are not verifying `Resources::apply()` here in this test. `updated1` was necessary because you need it as the expected value. src/tests/hierarchical_allocator_tests.cpp (lines 1376 - 1377) <https://reviews.apache.org/r/45964/#comment212686> Something more clear would be: ``` // The allocation should be the slave's original resources now that the volume is created and then destroyed. ``` src/tests/master_validation_tests.cpp (line 194) <https://reviews.apache.org/r/45964/#comment212772> It's not a volume? How about calling the test `UnshareableResource`? The *shareability* concept comes in handy here. src/tests/master_validation_tests.cpp (line 203) <https://reviews.apache.org/r/45964/#comment212773> s/Shareable/Shared/ src/tests/mesos.hpp (line 754) <https://reviews.apache.org/r/45964/#comment212774> The original comment was intended for the following three methods. Do you think we need to comment on each one? If so at least do it for LAUNCH as well and change plurals to singulars. src/tests/persistent_volume_tests.cpp (line 547) <https://reviews.apache.org/r/45964/#comment213415> It's unfortunate that we duplicate all the code from `PreparePersistentVolume` with only two lines of change. This applies to `AccessSharedPersistentVolume` as well. If we want to test whether shared volume works on the slave, we should create a test for the more interesting case: multiple tasks accessing the same persistent volume. This test would be both necessary and also would cover both "preparing" and "accessing" the shared persistent volume therefore we don't need to test them separately. src/tests/persistent_volume_tests.cpp (line 586) <https://reviews.apache.org/r/45964/#comment213414> Add a trailing comment: // Shared. src/tests/resources_tests.cpp (line 790) <https://reviews.apache.org/r/45964/#comment213416> This test should be much simpler: just create a persistent volume using `createPersistentVolume()` and verify the output of one copy and two copies, and that's it. src/tests/resources_tests.cpp (lines 2366 - 2367) <https://reviews.apache.org/r/45964/#comment213417> We don't use indentation style like this. The following would be fine: ``` Resource volume1 = createDiskResource( "200", "role", "1", "path", None(), true); ``` Here and below. src/tests/resources_tests.cpp (lines 2378 - 2379) <https://reviews.apache.org/r/45964/#comment213418> Ditto. src/tests/resources_tests.cpp (line 2390) <https://reviews.apache.org/r/45964/#comment213419> Also verify `nonShared()`? Plus, there is no operation (therefore shouldn't be in ResourcesOperationTest)? src/tests/resources_tests.cpp (line 2871) <https://reviews.apache.org/r/45964/#comment213420> This test came up before IIRC. I don't think we need this as the existing tests already have "multiple consumers". src/tests/resources_tests.cpp (line 2905) <https://reviews.apache.org/r/45964/#comment213421> This is the same as "FilterSharedPersistentVolumes" test above? We need only one and the one above is simpler. - Jiang Yan Xu On Aug. 24, 2016, 9:15 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45964/ > ----------------------------------------------------------- > > (Updated Aug. 24, 2016, 9:15 p.m.) > > > Review request for mesos and Jiang Yan Xu. > > > Bugs: MESOS-4324 > https://issues.apache.org/jira/browse/MESOS-4324 > > > Repository: mesos > > > Description > ------- > > Add unit tests for sharing of resources. > > > Diffs > ----- > > src/tests/hierarchical_allocator_tests.cpp > cbed333f497016fe2811f755028796012b41db77 > src/tests/master_validation_tests.cpp > 86b4b22350175af592876fd2f6d3fecca7acabce > src/tests/mesos.hpp 4cae54b4df906d4b7e8fe8d40d5b0ad59d260e6f > src/tests/persistent_volume_tests.cpp > a6f97c4bb5fb29d610c01255036095e2b30c44c5 > src/tests/resources_tests.cpp 4932d95f4e78cae764b89472373e13527b4354a2 > > Diff: https://reviews.apache.org/r/45964/diff/ > > > Testing > ------- > > Tests for shared resources added for allocator, resources and sorter. > Tests successful. > > > Thanks, > > Anindya Sinha > >
