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 64304: Enforced quota limit in the presence of unallocated reservations.
Date Tue, 12 Dec 2017 03:25:09 GMT

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


Fix it, then Ship it!




Looks good! Just some comments related to comments and naming, no code changes, so I can take
care of these for you and get this committed!


src/master/allocator/mesos/hierarchical.cpp
Lines 1596-1602 (patched)
<https://reviews.apache.org/r/64304/#comment272053>

    Looks like we could add the same structure to this comment as I suggested in the previous
review.



src/master/allocator/mesos/hierarchical.cpp
Line 1602 (original), 1640 (patched)
<https://reviews.apache.org/r/64304/#comment272048>

    a quantity?



src/master/allocator/mesos/hierarchical.cpp
Lines 1602-1608 (original), 1640-1646 (patched)
<https://reviews.apache.org/r/64304/#comment272050>

    Some symmetry here would be helpful:
    
    roleReservationQuantities vs 
    roleAllocatedReservationQuantities
    
    The maps:
    
    reservationScalarQuantities vs
    allocatedReservationScalarQuantites
    
    Probably it makes sense to be consistent with "quantities" vs "scalarQuantites"?



src/master/allocator/mesos/hierarchical.cpp
Line 1606 (original), 1644 (patched)
<https://reviews.apache.org/r/64304/#comment272049>

    a quantity?



src/master/allocator/mesos/hierarchical.cpp
Lines 1648-1656 (patched)
<https://reviews.apache.org/r/64304/#comment272052>

    Maybe a little bit more of an explanation of why we count reservations?
    
    ```
          // We charge a role against its quota by considering its
          // allocation as well as any unallocated reservations
          // since reservations are bound to the role. In other
          // words, we always consider reservations as consuming
          // quota, regardless of whether they are allocated.
          // The equation used here is:
          //
          //   Consumed Quota = reservations + unreserved allocation
          //                  = reservations + (allocation - allocated reservations)
          //
          // This is a __quantity__ with no meta-data.
          Resources resourcesChargedAgainstQuota =
            roleReservationScalarQuantities +
              (getQuotaRoleAllocatedResources(role) -
                   roleAllocatedReservationScalarQuantities);
    ```



src/master/allocator/mesos/hierarchical.cpp
Lines 1658-1663 (patched)
<https://reviews.apache.org/r/64304/#comment272054>

    How about?
    
    ```
          // If quota for the role is considered satisfied, then we only
          // further allocate reservations for the role.
          //
          // More precisely, we stop allocating unreserved resources if at
          // least one of the resource guarantees is considered consumed.
          // This technique prevents gaming of the quota allocation,
          // see MESOS-6432.
    ```
    
    I feel like "satisfied" is explained in the second paragraph, and this updates that paragram
to say "considered consumed" which matches up with the comment about what we consider consumed
against quota.



src/master/allocator/mesos/hierarchical.cpp
Lines 1682-1693 (original), 1732-1743 (patched)
<https://reviews.apache.org/r/64304/#comment272055>

    Looks good, thanks for updating this!



src/master/allocator/mesos/hierarchical.cpp
Lines 1791 (patched)
<https://reviews.apache.org/r/64304/#comment272057>

    // Update the tracking of allocated reservations.
    
    Seems less brittle to becoming stale if the variable name changes.



src/master/allocator/mesos/hierarchical.cpp
Lines 1800-1802 (patched)
<https://reviews.apache.org/r/64304/#comment272056>

    Just an observation, unlike the tracked map, this map will contain role entries with empty
resources. Don't think we need to write this down, just an inconsistency between the two that
may crop up later (e.g. if this map gets moved to a allocator global map).


- Benjamin Mahler


On Dec. 11, 2017, 4:29 a.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64304/
> -----------------------------------------------------------
> 
> (Updated Dec. 11, 2017, 4:29 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Michael Park.
> 
> 
> Bugs: MESOS-4527
>     https://issues.apache.org/jira/browse/MESOS-4527
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Enforced quota limit in the presence of unallocated reservations.
> Also modify the allocation process such that the first stage allocation
> is solely for quota roles while the second stage is solely for
> non-quota roles.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 9d8b6714d060f67d570c5653798e705248781869

> 
> 
> Diff: https://reviews.apache.org/r/64304/diff/6/
> 
> 
> Testing
> -------
> 
> make check.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


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