----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45961/#review146046 -----------------------------------------------------------
Comments mainly on tests (which I didn't look at earlier). src/master/allocator/sorter/drf/sorter.cpp (line 165) <https://reviews.apache.org/r/45961/#comment212864> At this point we don't know if the resources are **scalars** or not, they could be of other types. So `s/scalars/_resources/` ? src/master/allocator/sorter/drf/sorter.cpp (line 285) <https://reviews.apache.org/r/45961/#comment212872> Ditto about the name `scalars`. src/tests/master_validation_tests.cpp (line 609) <https://reviews.apache.org/r/45961/#comment212401> s/SharedPersistentVolumes/SharedPersistentVolumeInUse/ src/tests/master_validation_tests.cpp (lines 611 - 612) <https://reviews.apache.org/r/45961/#comment212402> s/cpus1/cpus/ s/mem1/mem/ src/tests/master_validation_tests.cpp (line 640) <https://reviews.apache.org/r/45961/#comment212395> We can just use `{}` here. src/tests/master_validation_tests.cpp (line 649) <https://reviews.apache.org/r/45961/#comment212396> Ditto. src/tests/master_validation_tests.cpp (lines 664 - 665) <https://reviews.apache.org/r/45961/#comment212400> Ditto. src/tests/master_validation_tests.cpp (lines 670 - 671) <https://reviews.apache.org/r/45961/#comment212399> Ditto. src/tests/sorter_tests.cpp (line 728) <https://reviews.apache.org/r/45961/#comment213405> I see that this is modeled after `SorterTest.DRFSorter` but in general shorter and more focused tests are preferred, especially the ones like this which require no setup. So here at least let's try to make it shorter by removing bits that are not strictly increasing our test coverage, especially given there's already `SorterTest.DRFSorter`. src/tests/sorter_tests.cpp (line 739) <https://reviews.apache.org/r/45961/#comment212431> ``` // 1000 'disk' in total (shared + non-shared). ``` src/tests/sorter_tests.cpp (lines 747 - 748) <https://reviews.apache.org/r/45961/#comment212442> Let's be consistent with the sequence in a block. Same as with client a: first `sorter.add("b");` then prepare b's resources and call `allocated()`. Here and below for other clients as well. src/tests/sorter_tests.cpp (line 751) <https://reviews.apache.org/r/45961/#comment213331> This would help me understand better. Here and below. Also capitalize it. ``` // Shares: a = .1 (dominant: disk), b = .06 (dominant: cpus). ``` src/tests/sorter_tests.cpp (lines 759 - 761) <https://reviews.apache.org/r/45961/#comment213337> src/tests/sorter_tests.cpp (lines 759 - 761) <https://reviews.apache.org/r/45961/#comment213349> We already have a, b and c, d looks to be just another simple client with only non-shared allocation and not all that interesting? Can we keep it simple and remove `d`? src/tests/sorter_tests.cpp (line 763) <https://reviews.apache.org/r/45961/#comment213346> Some shared resources specific comments? ``` // 'a' and 'c' share the same shared volume which are the the dominant resource for both. ``` src/tests/sorter_tests.cpp (lines 773 - 779) <https://reviews.apache.org/r/45961/#comment213347> Doesn't look like this block with `e` add much to the test coverage, remove it? src/tests/sorter_tests.cpp (line 781) <https://reviews.apache.org/r/45961/#comment213406> The rest of the test doesn't seem to have much to do with sharedness of resources directly so let's just make it clear: ``` // Verify other basic allocator methods work when shared resources are in the allocations. ``` Correspondingly, the first half of the test can be summarized as: ``` // Verify sort() works when shared resources are in the allocations. ``` - Jiang Yan Xu On Aug. 12, 2016, 5:57 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45961/ > ----------------------------------------------------------- > > (Updated Aug. 12, 2016, 5:57 p.m.) > > > Review request for mesos, Benjamin Mahler, Joris Van Remoortere, Michael > Park, and Jiang Yan Xu. > > > Bugs: MESOS-4431 > https://issues.apache.org/jira/browse/MESOS-4431 > > > Repository: mesos > > > Description > ------- > > o Each shared resource is accouted via its share count. This count is > updated based on the resource operations (such as add and subtract) > in various scenarios such as task launch and terminate at multiple > modules such as master, allocator, sorter, etc. > o Only allow DESTROY if there are no running or pending tasks using > the volume. However, if the volume is in a pending offer to one or > more frameworks, rescind that offer before processing the DESTROY. > o To allow multiple tasks to be launched in the same ACCEPT call > using the same shared resource, we update the allocator and > sorter with additional copies of shared resources to reflect the > true shared count of allocated shared resources. > > > Diffs > ----- > > src/common/resources.cpp 6b7af9179121efbdc5c29484eb042778bcea8288 > src/common/resources_utils.cpp 8e881a09500b0966b577e514077460f724868a8d > src/master/allocator/mesos/hierarchical.hpp > bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 > src/master/allocator/mesos/hierarchical.cpp > 234ef98529964a0b6d3f132426a6c8ccbb1263ee > src/master/allocator/sorter/drf/sorter.hpp > bc6bfb2d5d3b32d55be055a0514861b4e7d889bb > src/master/allocator/sorter/drf/sorter.cpp > ac85b327fc33d34246788e6a8c8bf5a486c61434 > src/master/http.cpp 52dd80b856cf2317c0b73ba54bf501696786088d > src/master/master.hpp 6decff6f4b9c3434de030fd5c06df4c683a7abad > src/master/master.cpp 0bd1a3490a86fede86a3f5f62ce4745b65aae258 > src/master/validation.hpp 43d876b00511d4ef652c2685d4950c994b5eef31 > src/master/validation.cpp af18e5aef3be59830b0a7b0235bbc739dba1029c > src/tests/master_validation_tests.cpp > 9eb82a569f7c48caa96d4d54a93b199ccac74385 > src/tests/sorter_tests.cpp 821e30d6574b045d25d4de4f7c3b8ac5346d3002 > src/v1/resources.cpp 03ee0cb0bb5abe7fc1ae4cb47c5b6dbcd8d11998 > > Diff: https://reviews.apache.org/r/45961/diff/ > > > Testing > ------- > > Tests successful. > > > Thanks, > > Anindya Sinha > >
