> On Jan. 19, 2016, 11:53 a.m., Guangya Liu wrote: > > src/tests/hierarchical_allocator_tests.cpp, lines 1817-1829 > > <https://reviews.apache.org/r/42289/diff/3/?file=1201483#file1201483line1817> > > > > What about make the test cases cover both `addSlave()` before and after > > `createFrameworkInfo`? > > Klaus Ma wrote: > The case after `createFrameworkInfo` is enough to cover both cases. > > Guangya Liu wrote: > Because this test is testing the cases that the resources added after > create framework can be also allocated to framework, I think that it is > better to keep `addSlave` cases `before` and `after` create framework to make > sure that the resources can be allocated to framework for both conditions. > > Alexander Rukletsov wrote: > If we add an agent *before* adding any frameworks, we may lay away > resources, but no allocations will be made. Currently we can't really look > into the allocator and check whether the resources have been actually laid > away. However, once we have at least one framework, allocations can be made. > Hence I would agree with Klaus that one test case is enough, but we want to > make sure people understand that the order is important and that we actually > test two things with one test. > > I'll think about how we can improve comments around the test. > > Guangya Liu wrote: > Do you mean `addSlave` and `createFramework` order? Does the order still > important after this patch?
It is important to prove the patch fixes the issue : ). - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42289/#review115143 ----------------------------------------------------------- On Jan. 19, 2016, 2:25 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, 2:25 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 > Mac & Ubuntu: ./bin/mesos-tests.sh > --gtest_filter="HierarchicalAllocatorTest.*" --verbose --gtest_repeat=100 > --gtest_shuffle > > > Thanks, > > Klaus Ma > >
