> On Jan. 19, 2016, 5:23 p.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? > > Alexander Rukletsov wrote: > Maybe even better, you can modify existing `QuotaAbsentFramework` test.
Yes, that's what I'm thinking :). Current patch passed Neil's test in Mac OS, I'll re-check it in Ubuntu for `./bin/mesos-tests.sh`; it seems perf did not support no-Linux system. - Klaus ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42289/#review115132 ----------------------------------------------------------- On Jan. 19, 2016, 6:55 p.m., Klaus Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42289/ > ----------------------------------------------------------- > > (Updated Jan. 19, 2016, 6:55 p.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 > ------- > > __Phenomenon__: > Quota doesn't allocate resources on slave joining. > > __Root Cause__: > 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. > > Refer to AlexR's comments for more detail: > https://issues.apache.org/jira/browse/MESOS-4102?focusedCommentId=15048495&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15048495 > > __Solution/Fix__: > Calcuated 'remainingClusterResources' by all activated slaves. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.cpp > 48acde69b1a2f305b568a7e322a58708063dd30a > src/tests/hierarchical_allocator_tests.cpp > 9362dd306497ba01e0f387c3862456cdcac6f863 > > Diff: https://reviews.apache.org/r/42289/diff/ > > > Testing > ------- > > make > make check > > > Thanks, > > Klaus Ma > >
