> On March 12, 2018, 3:29 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1610-1612 (patched)
> > <https://reviews.apache.org/r/66016/diff/2/?file=1973318#file1973318line1620>
> >
> >     Hm.. as an aside, looks like `Option::getOrElse` could be improved  to 
> > avoid copying in cases like this?
> >     
> >     E.g.
> >     
> >     ```
> >       T&& getOrElse(T&& _t) && { return std::move(isNone() ? _t : t); }
> >       T&& getOrElse(T&& _t) const && { return std::move(isNone() ? _t : t); 
> > }
> >     ```

Sounds good. Will add this in a follow up patch.


> On March 12, 2018, 3:29 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1616 (patched)
> > <https://reviews.apache.org/r/66016/diff/2/?file=1973318#file1973318line1626>
> >
> >     This should probably be renamed to s/Resources/ScalarQuantities/ to be 
> > consistent with your other naming? Want to do that in a separate patch?

Will do.


> On March 12, 2018, 3:29 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2002-2003 (original), 1972-1973 (patched)
> > <https://reviews.apache.org/r/66016/diff/2/?file=1973318#file1973318line2028>
> >
> >     Why isn't this:
> >     
> >     ```
> >     rolesConsumedQuotaScalarQuantites[role] +=
> >       newQuotaAllocationScalarQuantities;
> >     ```
> >     
> >     It's not obvious to me, are the two equivalent? The one included in the 
> > patch seems to be including resources that aren't part of the quota 
> > guarantee? Seems weird?

Both produce the correct result (version 1 of this patch actaully uses this 
formula). But I think `+= allocatedUnreserved` makes more sense. The way we 
calculate `rolesConsumedQuotaScalarQuantites` in the first place is by summing 
`reservations + allocation - allocated reservations` i.e. 
`quota.contains(rolesConsumedQuotaScalarQuantites)` was never true.

I have plan to persist `rolesConsumedQuotaScalarQuantites` for quota role 
across iterations. Consider a role updated with a new quota. `+= 
newQuotaAllocationScalarQuantities` would need to recalculate 
`rolesConsumedQuotaScalarQuantites` but `+= allocatedUnreserved` would be fine.

In a more general sense, each role has some (default) quota for all resources, 
consumed resources that only has default quota should still be counted as 
consumed quota.


- Meng


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


On March 12, 2018, 4:07 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66016/
> -----------------------------------------------------------
> 
> (Updated March 12, 2018, 4:07 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kapil Arya, Joseph Wu, Michael 
> Park, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The current allocation logic maintains several states
> such as allocated-reservation, unsatisfied-quota and etc.
> This patch refactors the logic so that we only need to
> maintain a per-quota-role map to track role consumed quota.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5d30d1d4d4bbb5431745f61b5318b39c5c3a7117 
> 
> 
> Diff: https://reviews.apache.org/r/66016/diff/3/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Reply via email to