----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42289/#review115132 -----------------------------------------------------------
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? src/master/allocator/mesos/hierarchical.cpp (lines 1227 - 1228) <https://reviews.apache.org/r/42289/#comment175992> Let's add a note here that we iterate over all slaves and why. How about // NOTE: We use all active agents and not just those visible in the current `allocate()` call so that frameworks in roles without quota are not unnecessarily deprived of resources. - Alexander Rukletsov 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 > >
