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 67827: Added a helper `stripIncapableResources` in the allocator.
Date Thu, 05 Jul 2018 22:16:40 GMT

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



Looks good, the second allocation loop is starting to look pretty succinct!

Just a few questions about the logic below


src/master/allocator/mesos/hierarchical.hpp
Lines 687 (patched)
<https://reviews.apache.org/r/67827/#comment288668>

    s/ based on the ..././
    
    Or this might be clearer?
    
    // Returns the subset of resources that the framework
    // is capable of being offered.



src/master/allocator/mesos/hierarchical.cpp
Lines 1768-1772 (original), 1768-1770 (patched)
<https://reviews.apache.org/r/67827/#comment288670>

    Hm.. do we need the if condition? Can't this just be:
    
    ```
    available -= offeredSharedResources.get(slaveId).getOrElse(Resources());
    ```



src/master/allocator/mesos/hierarchical.cpp
Line 1930 (original), 1912-1914 (patched)
<https://reviews.apache.org/r/67827/#comment288671>

    Hm.. this looks like a no-op change?



src/master/allocator/mesos/hierarchical.cpp
Lines 1998-2003 (original), 1982-1984 (patched)
<https://reviews.apache.org/r/67827/#comment288673>

    Ditto here



src/master/allocator/mesos/hierarchical.cpp
Line 2083 (original), 2044-2046 (patched)
<https://reviews.apache.org/r/67827/#comment288672>

    Ditto here



src/master/allocator/mesos/hierarchical.cpp
Lines 2660-2685 (patched)
<https://reviews.apache.org/r/67827/#comment288676>

    Should we re-write this now to just consistently use filter()?
    
    ```
    Resources HierarchicalAllocatorProcess::stripIncapableResources(
        const Resources& resources, // Can take a const ref now?
        const protobuf::framework::Capabilities& frameworkCapabilities) const
    {
      return resources.filter([&](const Resource& resource) {
        if (!frameworkCapabilities.sharedResources &&
            Resources::isShared(resource)) {
          return false;
        }
        
        if (!frameworkCapabilities.revocableResources &&
            Resources::isRevocable(resource)) {
          return false;
        }
        
        // When reservation refinements are present, old frameworks without the
        // RESERVATION_REFINEMENT capability won't be able to understand the
        // new format. While it's possible to translate the refined reservations
        // into the old format by "hiding" the intermediate reservations in the
        // "stack", this leads to ambiguity when processing RESERVE / UNRESERVE
        // operations. This is due to the loss of information when we drop the
        // intermediate reservations. Therefore, for now we simply filter out
        // resources with refined reservations if the framework does not have
        // the capability.
        if (!frameworkCapabilities.reservationRefinement &&
            Resources::hasRefinedReservations(resource)) {
          return false;
        }
        
        return true;
      });
    }
    ```
    
    I think this approach avoids adding extra copyies as we add more cases?


- Benjamin Mahler


On July 4, 2018, 1:01 a.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67827/
> -----------------------------------------------------------
> 
> (Updated July 4, 2018, 1:01 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8916
>     https://issues.apache.org/jira/browse/MESOS-8916
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This helper removes any resources that the framework is not
> capable of receiving based on the given framework capability.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.hpp 0f6c0e96a105c64465d3f5db4ff663d8fdfe7e26

>   src/master/allocator/mesos/hierarchical.cpp 5a6cd3d2fc5bdbaaee2d05b9be9e83d4107c749b

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


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