> On Jan. 19, 2016, 9:23 a.m., Alexander Rukletsov wrote: > > Let's tweak some wording and testing and we are good to go! > > > > I liked the initial summary more. IMO a patch should describe the solution, > > and not the problem. It's quite opposite for JIRA tickets, hence I'm > > convinced patches and tickets should not share the same title. How about > > "Calcuated 'remainingClusterResources' using all available agents." > > > > I also think it won't hurt to extend the description and explain why we > > need this change. How about > > "Event-triggered allocations do not include all available agents. If we > > calculate remaining resources in the cluster using the partial view, > > we may overlook already laid away resources for quota'ed roles and lay > > away more. Hence we may unnecessarily deprive non-quota'ed frameworks > > of resources." > > > > Changes touching allocator are vulnerable to races, especially in tests. > > Please extend the testing (and mention this in the "Testing Done" section) > > with goodies like `GTEST_FILTER="*HierarchicalAllocatorTest*" > > ./bin/mesos-tests.sh --gtest_shuffle --gtest-repeat`. > > > > Let's add a test for this change. Ideally the test should fail without the > > change and pass it with the change. I think Neil has already provided the > > test in the ticket, could you please include it?
Maybe even better, you can modify existing `QuotaAbsentFramework` test. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42289/#review115132 ----------------------------------------------------------- On Jan. 19, 2016, 8:50 a.m., Klaus Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42289/ > ----------------------------------------------------------- > > (Updated Jan. 19, 2016, 8:50 a.m.) > > > Review request for mesos, Alexander Rukletsov, Ben Mahler, Joris Van > Remoortere, and Neil Conway. > > > Bugs: MESOS-4102 > https://issues.apache.org/jira/browse/MESOS-4102 > > > Repository: mesos > > > Description > ------- > > Calcuated 'remainingClusterResources' by all activated slaves. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.cpp > 48acde69b1a2f305b568a7e322a58708063dd30a > > Diff: https://reviews.apache.org/r/42289/diff/ > > > Testing > ------- > > make > make check > > > Thanks, > > Klaus Ma > >
