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


Fix it, then Ship it!





src/master/master.hpp
Lines 2730 (patched)
<https://reviews.apache.org/r/71029/#comment303668>

    `totalUsedResources` here also includes the executor resources which are 
not managed by the allocator. Do we want to include that in the consumption?
    
    If no, then the consumption is not going to be consistent with framework 
stats. But if yes, since the consumption will be compared with quota (both from 
the user perspective as well as internally in the future), that means when 
operator configures the quota, they need to take executor resources into 
account. I am not sure if that's good user experience.
    
    For simplicity, I am OK with leaving it as is now. But let's document the 
rationale and/or any todos.
    
    Also please update the description for this.



src/master/master.hpp
Lines 2743-2752 (patched)
<https://reviews.apache.org/r/71029/#comment303671>

    how about total_reservaiton - used_reservation:
    
    ```
    ResourceQuantities unallocatedReservation;
    
     foreachvalue (Slave* slave, master->slaves.registered) {
       ResourceQuantities totalReservation =
         ResourceQuantities::fromScalarResources(
           slave->totalResources.filter(reservedToRoleSubtree).scalars());
           
       ResourceQuantities usedReservation =
         ResourceQuantities::fromScalarResources(
           slave->usedResources.filter(reservedToRoleSubtree).scalars());
       
       unallocatedReservation += totalReservation - usedReservation;
     }
    
    ```
    
    Should also be more performant to use ResourceQuantities and no need to 
clear the allocation info.



src/master/master.hpp
Lines 2758 (patched)
<https://reviews.apache.org/r/71029/#comment303672>

    Nits, how about avoiding the addition and the two variables by just having 
a `consumption`:
    
    ```
    // Quota consumption = allocation + unallocated reservation
    
    ResourceQuantities consumption;
    
    // Add allocation
    
    ....
    
    // Add unallocated reservation
    ...
    
    return consumption;
    
    ```


- Meng Zhu


On July 8, 2019, 11:07 a.m., Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71029/
> -----------------------------------------------------------
> 
> (Updated July 8, 2019, 11:07 a.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Meng Zhu.
> 
> 
> Bugs: MESOS-9871
>     https://issues.apache.org/jira/browse/MESOS-9871
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Quota consumption is computed as follows:
> 
>   Allocation + Unallocated Reservation ==
>   Reservations + Unreserved Allocation
> 
> That is, reservations count towards quota regardless of whether
> they're allocated. Allocation counts towards quota. Offered resources
> *do not* count towards quota, this is to (1) provide stability of the
> quota consumption metrics in the face of offers flowing in and out,
> and (2) to ensure that we treat offers as rescindable and therefore
> not yet "counting" towards quota. Also, if in the future schedulers
> are offered more than their quota to improve their choices, counting
> offered resources as quota consumption will be problematic.
> 
> 
> Diffs
> -----
> 
>   src/master/framework.cpp c35de2574ef683a9f397a957422fdbf8fa751313 
>   src/master/master.hpp 23dafe746b6f9b3d70ad7220f54c4d49068b8af8 
> 
> 
> Diff: https://reviews.apache.org/r/71029/diff/1/
> 
> 
> Testing
> -------
> 
> Added a test in a subsequent patch.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Reply via email to