mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 70393: Fixed incorrect quota headroom and consumption calculations.
Date Fri, 05 Apr 2019 02:45:57 GMT


> On April 5, 2019, 12:59 a.m., Meng Zhu wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1714-1715 (patched)
> > <https://reviews.apache.org/r/70393/diff/2/?file=2137549#file2137549line1714>
> >
> >     I think this could just be:
> >     
> >     ```
> >     const string& topLevelRole = role.substr(0, role.find('/'));
> >     ```
> >     
> >     I do not have strong opinion on this, I will leave this up to you.

Hm.. intresting, it's a bit "clever" but also, it means that in the typical case today of
the role being already top level, we would do an unnecessary copy (potential malloc) vs the
current approach in the patch which only copies if non-top level? (of course, the additional
scan and branch have costs too.. :)).

I think since the reader reasons about it as two cases (non top level and top level), it reads
a bit easier with the condition.


> On April 5, 2019, 12:59 a.m., Meng Zhu wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1711-1725 (original), 1722-1736 (patched)
> > <https://reviews.apache.org/r/70393/diff/2/?file=2137549#file2137549line1722>
> >
> >     We should be able to combine these two?
> >     
> >     ```
> >     // Then add allocated unreserved resource quantities.
> >     if (roleSorter->contains(role)) {
> >       foreachvalue (const Resources& resources, roleSorter->allocation(role))
{
> >         rolesConsumedQuota[topLevelRole] += ResourceQuantities::fromScalarResources(resources.unreserved().nonRevocable().scalars());
> >       }
> >     }
> >     
> >     ```

Yes, good point and nice and succict!

This means I should probably adjust the formula above to reflect that we now just use the
first form rather than the expanded form with the subtraction.


- Benjamin


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


On April 4, 2019, 11 p.m., Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70393/
> -----------------------------------------------------------
> 
> (Updated April 4, 2019, 11 p.m.)
> 
> 
> Review request for mesos and Meng Zhu.
> 
> 
> Bugs: MESOS-9688 and MESOS-9691
>     https://issues.apache.org/jira/browse/MESOS-9688
>     https://issues.apache.org/jira/browse/MESOS-9691
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> There was an invalid assumption that the sorter returns
> hierarchically aggregated allocation information, whereas
> it actually returns the "." internal node information. This
> patch adjusts the allocator code accordingly.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 1fd8b021c9954c37533dc193b3148def5ff53071

> 
> 
> Diff: https://reviews.apache.org/r/70393/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message