-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39401/#review103958
-----------------------------------------------------------



src/master/allocator/mesos/hierarchical.cpp (line 996)
<https://reviews.apache.org/r/39401/#comment162066>

    These newly added code makes allocate() a huge method (more than 200 
lines), maybe move these codes into a separate method?



src/master/allocator/mesos/hierarchical.cpp (lines 1017 - 1020)
<https://reviews.apache.org/r/39401/#comment162085>

    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.



src/master/allocator/mesos/hierarchical.cpp (lines 1055 - 1057)
<https://reviews.apache.org/r/39401/#comment162144>

    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.



src/master/allocator/mesos/hierarchical.cpp (line 1058)
<https://reviews.apache.org/r/39401/#comment162132>

    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 "+="?



src/master/allocator/mesos/hierarchical.cpp (line 1097)
<https://reviews.apache.org/r/39401/#comment162129>

    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;
    }
    ```


- Qian Zhang


On Oct. 24, 2015, 12:38 a.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39401/
> -----------------------------------------------------------
> 
> (Updated Oct. 24, 2015, 12:38 a.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
> 
>

Reply via email to