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 67827: Added a helper `stripIncapableResources` in the allocator.
Date Tue, 17 Jul 2018 21:33:08 GMT


> On July 5, 2018, 3:16 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.hpp
> > Lines 687 (patched)
> > <https://reviews.apache.org/r/67827/diff/1/?file=2049474#file2049474line687>
> >
> >     s/ based on the ..././
> >     
> >     Or this might be clearer?
> >     
> >     // Returns the subset of resources that the framework
> >     // is capable of being offered.

I want to emphasize this function only considers framework capability, things like offer filters
and etc. are not considered. This is why I think without the "based on the given framework
capability" there could be doubt.


> On July 5, 2018, 3:16 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2660-2685 (patched)
> > <https://reviews.apache.org/r/67827/diff/1/?file=2049475#file2049475line2701>
> >
> >     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?

Good point.


- Meng


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


On July 17, 2018, 2:32 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67827/
> -----------------------------------------------------------
> 
> (Updated July 17, 2018, 2:32 p.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 1f1a73fc2308301cfed6ea1131e09fbf3393dfc7

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


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