> On Oct. 26, 2015, 8:27 a.m., Qian Zhang wrote: > > src/master/allocator/mesos/hierarchical.cpp, line 996 > > <https://reviews.apache.org/r/39401/diff/5/?file=1105060#file1105060line996> > > > > These newly added code makes allocate() a huge method (more than 200 > > lines), maybe move these codes into a separate method?
Absolutely! The reason why it's not done is because we have already planned (but not yet scheduled) an allocator refactoring. Let me add a `TODO` for now in order to increase the pressure on ourselves ; ). > On Oct. 26, 2015, 8:27 a.m., Qian Zhang wrote: > > src/master/allocator/mesos/hierarchical.cpp, lines 1017-1020 > > <https://reviews.apache.org/r/39401/diff/5/?file=1105060#file1105060line1017> > > > > Why do we put these code inside the framework sorters foreach loop? I > > do not see it is related to framework. > > If we really want to put these code here, then I think we also need to > > recalculate roleAllocatedResources every time when we allocate some > > resources to a framework of the role, and once the quota for the role is > > satifised, break. There can be multiple frameworks in a role, hence quota may get satisfied after we allocate resources to some frameworks. > then I think we also need to recalculate roleAllocatedResources every time > when we allocate some resources to a framework But we do that at the end of the loop, right? > On Oct. 26, 2015, 8:27 a.m., Qian Zhang wrote: > > src/master/allocator/mesos/hierarchical.cpp, lines 1055-1057 > > <https://reviews.apache.org/r/39401/diff/5/?file=1105060#file1105060line1055> > > > > Since we know the guarentee of each quota'ed role and the gap between > > it and role's current allocation, I think it might be better to do the > > "find-grained" allocation, i.e., only allocate resources to fill the gap. > > Otherwise, we may run into the situation that we allocate too much resource > > to satisfy a role's guarentee but another role's guarentee can not be > > satisfied. Yep, this can be the case and it was a hot debate during the design phase. I have decided to choose this approach because it's follows the least surprise principle for people who already familiar with DRF. I think we'll have to revisit the strategy anyway and (most probably) introduce `Quota.limit`. I think this is fine for MVP. > On Oct. 26, 2015, 8:27 a.m., Qian Zhang wrote: > > src/master/allocator/mesos/hierarchical.cpp, line 1058 > > <https://reviews.apache.org/r/39401/diff/5/?file=1105060#file1105060line1058> > > > > We also have this code ```offerable[frameworkId][slaveId] = > > resources;``` in the following DRF allocation stage, so that means for a > > framework, the resources we allocate to it here will be overwritten by the > > the resources we allocate to it in DRF allocation stage? This seems not > > correct, maybe change the code in DRF allocation stage from "=" to "+="? Good catch! > On Oct. 26, 2015, 8:27 a.m., Qian Zhang wrote: > > src/master/allocator/mesos/hierarchical.cpp, line 1097 > > <https://reviews.apache.org/r/39401/diff/5/?file=1105060#file1105060line1097> > > > > If we *continue* from here, that means the following DRF allocation > > stage will be skipped? I think that is not what we want. So I guess it > > should be: > > ``` > > if (allocatable(resources)) { > > // Lay aside the resources. > > laidAsideResources[slaveId] = resources; > > slaves[slaveId].allocated += resources; > > } > > ``` I think you're right. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39401/#review103958 ----------------------------------------------------------- On Oct. 23, 2015, 4:38 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39401/ > ----------------------------------------------------------- > > (Updated Oct. 23, 2015, 4:38 p.m.) > > > Review request for mesos, Bernd Mathiske, Joerg Schad, and Joris Van > Remoortere. > > > Bugs: MESOS-3718 > https://issues.apache.org/jira/browse/MESOS-3718 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.cpp > f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 > > Diff: https://reviews.apache.org/r/39401/diff/ > > > Testing > ------- > > make check (Mac OS X 10.10.4) > > > Thanks, > > Alexander Rukletsov > >
