----------------------------------------------------------- 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 > >
