mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Meng Zhu <m...@mesosphere.io>
Subject Re: Review Request 70393: Fixed incorrect quota headroom and consumption calculations.
Date Thu, 04 Apr 2019 21:43:10 GMT

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




src/master/allocator/mesos/hierarchical.cpp
Lines 1675-1676 (original), 1675-1676 (patched)
<https://reviews.apache.org/r/70393/#comment300570>

    I think this comment change is not necessary. The top-level only tracking is based on
the fact that only top-level roles can have non-default quota atm.



src/master/allocator/mesos/hierarchical.cpp
Line 1708 (original), 1712-1722 (patched)
<https://reviews.apache.org/r/70393/#comment300581>

    I am confused
    
    Couldn't we just do
    
    ```
      foreachkey (const string& role, quotaGuarantees) {
        rolesConsumedQuota[role] +=
          reservationScalarQuantities.get(role).getOrElse(ResourceQuantities());
      }
    
    ```
    
    After your previous patch where you made `reservationScalarQuantities` hierarchical?



src/master/allocator/mesos/hierarchical.cpp
Lines 1713-1714 (patched)
<https://reviews.apache.org/r/70393/#comment300578>

    We are copying the whole role string here. Do you have new benchmark result regarding
the performance impact?
    
    If so, we need to do some optimizations e.g. have a `if` clause where we do not need to
copy the string in the top role case (common case atm)? 
    
    And for subroles case, we could just use `find("/")` to just copy the first part?
    
    In fact, we could do it now:
    
    ```
    string toLevelRole = role.substr(0, role.find("/"));
    
    ```



src/master/allocator/mesos/hierarchical.cpp
Lines 1725-1726 (patched)
<https://reviews.apache.org/r/70393/#comment300582>

    ditto here regarding the toplevelrole copy.



src/master/allocator/mesos/hierarchical.cpp
Line 1710 (original), 1732 (patched)
<https://reviews.apache.org/r/70393/#comment300583>

    Move this comment above the loop?


- Meng Zhu


On April 4, 2019, noon, Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70393/
> -----------------------------------------------------------
> 
> (Updated April 4, 2019, noon)
> 
> 
> 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/1/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>


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