This is an automated email from the ASF dual-hosted git repository. mzhu pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mesos.git
commit ef26c536f89c76ec2752299096bc5f5538753ed2 Author: Meng Zhu <[email protected]> AuthorDate: Wed Mar 27 17:17:32 2019 -0700 Used `ResourceQuantities` in the sorter when appropriate. Replaced `Resources` with `ResourceQuantities` when appropriate in the sorter. This simplifies the code and improves performance. Review: https://reviews.apache.org/r/70330 --- src/master/allocator/mesos/hierarchical.cpp | 29 ++------- src/master/allocator/sorter/drf/sorter.cpp | 27 +++++---- src/master/allocator/sorter/drf/sorter.hpp | 84 +++++++++------------------ src/master/allocator/sorter/random/sorter.cpp | 27 +++++---- src/master/allocator/sorter/random/sorter.hpp | 84 +++++++++------------------ src/master/allocator/sorter/sorter.hpp | 11 ++-- src/tests/sorter_tests.cpp | 10 ++-- 7 files changed, 98 insertions(+), 174 deletions(-) diff --git a/src/master/allocator/mesos/hierarchical.cpp b/src/master/allocator/mesos/hierarchical.cpp index 047e799..b9bddd3 100644 --- a/src/master/allocator/mesos/hierarchical.cpp +++ b/src/master/allocator/mesos/hierarchical.cpp @@ -1708,11 +1708,9 @@ void HierarchicalAllocatorProcess::__allocate() // Then add allocated resource _quantities_ . // NOTE: Revocable resources are excluded in `quotaRoleSorter`. - // - // TODO(mzhu): Update sorter to return `ResourceQuantities` directly. CHECK(quotaGuarantees.contains(role)); - rolesConsumedQuota[role] += ResourceQuantities::fromScalarResources( - quotaRoleSorter->allocationScalarQuantities(role)); + rolesConsumedQuota[role] += + quotaRoleSorter->allocationScalarQuantities(role); // Lastly subtract allocated reservations on each agent. foreachvalue ( @@ -1753,18 +1751,11 @@ void HierarchicalAllocatorProcess::__allocate() // allocated resources - // unallocated reservations - // unallocated revocable resources - // - // TODO(mzhu): Update sorter to return `ResourceQuantities` directly. - ResourceQuantities availableHeadroom = - ResourceQuantities::fromScalarResources( - roleSorter->totalScalarQuantities()); + ResourceQuantities availableHeadroom = roleSorter->totalScalarQuantities(); // Subtract allocated resources from the total. - // - // TODO(mzhu): Update sorter to return `ResourceQuantities` directly. foreachkey (const string& role, roles) { - availableHeadroom -= ResourceQuantities::fromScalarResources( - roleSorter->allocationScalarQuantities(role)); + availableHeadroom -= roleSorter->allocationScalarQuantities(role); } // Calculate total allocated reservations. Note that we need to ensure @@ -2457,11 +2448,7 @@ double HierarchicalAllocatorProcess::_resources_offered_or_allocated( double HierarchicalAllocatorProcess::_resources_total( const string& resource) { - Option<Value::Scalar> total = - roleSorter->totalScalarQuantities() - .get<Value::Scalar>(resource); - - return total.isSome() ? total->value() : 0; + return roleSorter->totalScalarQuantities().get(resource).value(); } @@ -2475,11 +2462,7 @@ double HierarchicalAllocatorProcess::_quota_allocated( return 0.; } - Option<Value::Scalar> used = - roleSorter->allocationScalarQuantities(role) - .get<Value::Scalar>(resource); - - return used.isSome() ? used->value() : 0; + return roleSorter->allocationScalarQuantities(role).get(resource).value(); } diff --git a/src/master/allocator/sorter/drf/sorter.cpp b/src/master/allocator/sorter/drf/sorter.cpp index 43c9767..a76888d 100644 --- a/src/master/allocator/sorter/drf/sorter.cpp +++ b/src/master/allocator/sorter/drf/sorter.cpp @@ -445,11 +445,11 @@ const hashmap<SlaveID, Resources>& DRFSorter::allocation( } -const Resources& DRFSorter::allocationScalarQuantities( +const ResourceQuantities& DRFSorter::allocationScalarQuantities( const string& clientPath) const { const Node* client = CHECK_NOTNULL(find(clientPath)); - return client->allocation.scalarQuantities; + return client->allocation.totals; } @@ -493,9 +493,9 @@ Resources DRFSorter::allocation( } -const Resources& DRFSorter::totalScalarQuantities() const +const ResourceQuantities& DRFSorter::totalScalarQuantities() const { - return total_.scalarQuantities; + return total_.totals; } @@ -511,11 +511,11 @@ void DRFSorter::add(const SlaveID& slaveId, const Resources& resources) total_.resources[slaveId] += resources; - const Resources scalarQuantities = - (resources.nonShared() + newShared).createStrippedScalarQuantity(); + const ResourceQuantities scalarQuantities = + ResourceQuantities::fromScalarResources( + (resources.nonShared() + newShared).scalars()); - total_.scalarQuantities += scalarQuantities; - total_.totals += ResourceQuantities::fromScalarResources(scalarQuantities); + total_.totals += scalarQuantities; // We have to recalculate all shares when the total resources // change, but we put it off until `sort` is called so that if @@ -542,13 +542,12 @@ void DRFSorter::remove(const SlaveID& slaveId, const Resources& resources) return !total_.resources[slaveId].contains(resource); }); - const Resources scalarQuantities = - (resources.nonShared() + absentShared).createStrippedScalarQuantity(); + const ResourceQuantities scalarQuantities = + ResourceQuantities::fromScalarResources( + (resources.nonShared() + absentShared).scalars()); - CHECK(total_.scalarQuantities.contains(scalarQuantities)); - total_.scalarQuantities -= scalarQuantities; - - total_.totals -= ResourceQuantities::fromScalarResources(scalarQuantities); + CHECK(total_.totals.contains(scalarQuantities)); + total_.totals -= scalarQuantities; if (total_.resources[slaveId].empty()) { total_.resources.erase(slaveId); diff --git a/src/master/allocator/sorter/drf/sorter.hpp b/src/master/allocator/sorter/drf/sorter.hpp index 75f90f3..9e4d036 100644 --- a/src/master/allocator/sorter/drf/sorter.hpp +++ b/src/master/allocator/sorter/drf/sorter.hpp @@ -86,7 +86,7 @@ public: const hashmap<SlaveID, Resources>& allocation( const std::string& clientPath) const override; - const Resources& allocationScalarQuantities( + const ResourceQuantities& allocationScalarQuantities( const std::string& clientPath) const override; hashmap<std::string, Resources> allocation( @@ -96,7 +96,7 @@ public: const std::string& clientPath, const SlaveID& slaveId) const override; - const Resources& totalScalarQuantities() const override; + const ResourceQuantities& totalScalarQuantities() const override; void add(const SlaveID& slaveId, const Resources& resources) override; @@ -157,26 +157,10 @@ private: // number of copies in the sorter. hashmap<SlaveID, Resources> resources; - // NOTE: Scalars can be safely aggregated across slaves. We keep - // that to speed up the calculation of shares. See MESOS-2891 for - // the reasons why we want to do that. - // - // NOTE: We omit information about dynamic reservations and - // persistent volumes here to enable resources to be aggregated - // across slaves more effectively. See MESOS-4833 for more - // information. - // - // Sharedness info is also stripped out when resource identities - // are omitted because sharedness inherently refers to the - // identities of resources and not quantities. - Resources scalarQuantities; - - // To improve the performance of calculating shares, we store - // a redundant but more efficient version of `scalarQuantities`. - // See MESOS-4694. - // - // TODO(bmahler): Can we remove `scalarQuantities` in favor of - // using this type whenever scalar quantities are needed? + // We keep the aggregated scalar resource quantities to speed + // up share calculation. Note, resources shared count are ignored. + // Because sharedness inherently refers to the identities of resources + // and not quantities. ResourceQuantities totals; } total_; @@ -338,13 +322,12 @@ struct DRFSorter::Node return !resources[slaveId].contains(resource); }); - const Resources quantitiesToAdd = - (toAdd.nonShared() + sharedToAdd).createStrippedScalarQuantity(); + const ResourceQuantities quantitiesToAdd = + ResourceQuantities::fromScalarResources( + (toAdd.nonShared() + sharedToAdd).scalars()); resources[slaveId] += toAdd; - scalarQuantities += quantitiesToAdd; - - totals += ResourceQuantities::fromScalarResources(quantitiesToAdd); + totals += quantitiesToAdd; count++; } @@ -365,15 +348,14 @@ struct DRFSorter::Node return !resources[slaveId].contains(resource); }); - const Resources quantitiesToRemove = - (toRemove.nonShared() + sharedToRemove).createStrippedScalarQuantity(); + const ResourceQuantities quantitiesToRemove = + ResourceQuantities::fromScalarResources( + (toRemove.nonShared() + sharedToRemove).scalars()); - totals -= ResourceQuantities::fromScalarResources(quantitiesToRemove); + CHECK(totals.contains(quantitiesToRemove)) + << totals << " does not contain " << quantitiesToRemove; - CHECK(scalarQuantities.contains(quantitiesToRemove)) - << scalarQuantities << " does not contain " << quantitiesToRemove; - - scalarQuantities -= quantitiesToRemove; + totals -= quantitiesToRemove; if (resources[slaveId].empty()) { resources.erase(slaveId); @@ -385,27 +367,24 @@ struct DRFSorter::Node const Resources& oldAllocation, const Resources& newAllocation) { - const Resources oldAllocationQuantity = - oldAllocation.createStrippedScalarQuantity(); - const Resources newAllocationQuantity = - newAllocation.createStrippedScalarQuantity(); + const ResourceQuantities oldAllocationQuantities = + ResourceQuantities::fromScalarResources(oldAllocation.scalars()); + const ResourceQuantities newAllocationQuantities = + ResourceQuantities::fromScalarResources(newAllocation.scalars()); CHECK(resources.contains(slaveId)); CHECK(resources[slaveId].contains(oldAllocation)) << "Resources " << resources[slaveId] << " at agent " << slaveId << " does not contain " << oldAllocation; - CHECK(scalarQuantities.contains(oldAllocationQuantity)) - << scalarQuantities << " does not contain " << oldAllocationQuantity; + CHECK(totals.contains(oldAllocationQuantities)) + << totals << " does not contain " << oldAllocationQuantities; resources[slaveId] -= oldAllocation; resources[slaveId] += newAllocation; - scalarQuantities -= oldAllocationQuantity; - scalarQuantities += newAllocationQuantity; - - totals -= ResourceQuantities::fromScalarResources(oldAllocationQuantity); - totals += ResourceQuantities::fromScalarResources(newAllocationQuantity); + totals -= oldAllocationQuantities; + totals += newAllocationQuantities; } // We store the number of times this client has been chosen for @@ -423,17 +402,10 @@ struct DRFSorter::Node // not been recovered from) a specific client. hashmap<SlaveID, Resources> resources; - // Similarly, we aggregate scalars across slaves and omit information - // about dynamic reservations, persistent volumes and sharedness of - // the corresponding resource. See notes above. - Resources scalarQuantities; - - // To improve the performance of calculating shares, we store - // a redundant but more efficient version of `scalarQuantities`. - // See MESOS-4694. - // - // TODO(bmahler): Can we remove `scalarQuantities` in favor of - // using this type whenever scalar quantities are needed? + // We keep the aggregated scalar resource quantities to speed + // up share calculation. Note, resources shared count are ignored. + // Because sharedness inherently refers to the identities of resources + // and not quantities. ResourceQuantities totals; } allocation; diff --git a/src/master/allocator/sorter/random/sorter.cpp b/src/master/allocator/sorter/random/sorter.cpp index 6fcfc41..8499c69 100644 --- a/src/master/allocator/sorter/random/sorter.cpp +++ b/src/master/allocator/sorter/random/sorter.cpp @@ -369,11 +369,11 @@ const hashmap<SlaveID, Resources>& RandomSorter::allocation( } -const Resources& RandomSorter::allocationScalarQuantities( +const ResourceQuantities& RandomSorter::allocationScalarQuantities( const string& clientPath) const { const Node* client = CHECK_NOTNULL(find(clientPath)); - return client->allocation.scalarQuantities; + return client->allocation.totals; } @@ -418,9 +418,9 @@ Resources RandomSorter::allocation( } -const Resources& RandomSorter::totalScalarQuantities() const +const ResourceQuantities& RandomSorter::totalScalarQuantities() const { - return total_.scalarQuantities; + return total_.totals; } @@ -436,11 +436,11 @@ void RandomSorter::add(const SlaveID& slaveId, const Resources& resources) total_.resources[slaveId] += resources; - const Resources scalarQuantities = - (resources.nonShared() + newShared).createStrippedScalarQuantity(); + const ResourceQuantities scalarQuantities = + ResourceQuantities::fromScalarResources( + (resources.nonShared() + newShared).scalars()); - total_.scalarQuantities += scalarQuantities; - total_.totals += ResourceQuantities::fromScalarResources(scalarQuantities); + total_.totals += scalarQuantities; } } @@ -461,13 +461,12 @@ void RandomSorter::remove(const SlaveID& slaveId, const Resources& resources) return !total_.resources[slaveId].contains(resource); }); - const Resources scalarQuantities = - (resources.nonShared() + absentShared).createStrippedScalarQuantity(); + const ResourceQuantities scalarQuantities = + ResourceQuantities::fromScalarResources( + (resources.nonShared() + absentShared).scalars()); - CHECK(total_.scalarQuantities.contains(scalarQuantities)); - total_.scalarQuantities -= scalarQuantities; - - total_.totals -= ResourceQuantities::fromScalarResources(scalarQuantities); + CHECK(total_.totals.contains(scalarQuantities)); + total_.totals -= scalarQuantities; if (total_.resources[slaveId].empty()) { total_.resources.erase(slaveId); diff --git a/src/master/allocator/sorter/random/sorter.hpp b/src/master/allocator/sorter/random/sorter.hpp index 2031cb2..513abf3 100644 --- a/src/master/allocator/sorter/random/sorter.hpp +++ b/src/master/allocator/sorter/random/sorter.hpp @@ -85,7 +85,7 @@ public: const hashmap<SlaveID, Resources>& allocation( const std::string& clientPath) const override; - const Resources& allocationScalarQuantities( + const ResourceQuantities& allocationScalarQuantities( const std::string& clientPath) const override; hashmap<std::string, Resources> allocation( @@ -95,7 +95,7 @@ public: const std::string& clientPath, const SlaveID& slaveId) const override; - const Resources& totalScalarQuantities() const override; + const ResourceQuantities& totalScalarQuantities() const override; void add(const SlaveID& slaveId, const Resources& resources) override; @@ -155,26 +155,10 @@ private: // number of copies in the sorter. hashmap<SlaveID, Resources> resources; - // NOTE: Scalars can be safely aggregated across slaves. We keep - // that to speed up the calculation of shares. See MESOS-2891 for - // the reasons why we want to do that. - // - // NOTE: We omit information about dynamic reservations and - // persistent volumes here to enable resources to be aggregated - // across slaves more effectively. See MESOS-4833 for more - // information. - // - // Sharedness info is also stripped out when resource identities - // are omitted because sharedness inherently refers to the - // identities of resources and not quantities. - Resources scalarQuantities; - - // To improve the performance of calculating shares, we store - // a redundant but more efficient version of `scalarQuantities`. - // See MESOS-4694. - // - // TODO(bmahler): Can we remove `scalarQuantities` in favor of - // using this type whenever scalar quantities are needed? + // We keep the aggregated scalar resource quantities to speed + // up share calculation. Note, resources shared count are ignored. + // Because sharedness inherently refers to the identities of resources + // and not quantities. ResourceQuantities totals; } total_; }; @@ -325,13 +309,12 @@ struct RandomSorter::Node return !resources[slaveId].contains(resource); }); - const Resources quantitiesToAdd = - (toAdd.nonShared() + sharedToAdd).createStrippedScalarQuantity(); + const ResourceQuantities quantitiesToAdd = + ResourceQuantities::fromScalarResources( + (toAdd.nonShared() + sharedToAdd).scalars()); resources[slaveId] += toAdd; - scalarQuantities += quantitiesToAdd; - - totals += ResourceQuantities::fromScalarResources(quantitiesToAdd); + totals += quantitiesToAdd; } void subtract(const SlaveID& slaveId, const Resources& toRemove) @@ -350,15 +333,14 @@ struct RandomSorter::Node return !resources[slaveId].contains(resource); }); - const Resources quantitiesToRemove = - (toRemove.nonShared() + sharedToRemove).createStrippedScalarQuantity(); + const ResourceQuantities quantitiesToRemove = + ResourceQuantities::fromScalarResources( + (toRemove.nonShared() + sharedToRemove).scalars()); - totals -= ResourceQuantities::fromScalarResources(quantitiesToRemove); + CHECK(totals.contains(quantitiesToRemove)) + << totals << " does not contain " << quantitiesToRemove; - CHECK(scalarQuantities.contains(quantitiesToRemove)) - << scalarQuantities << " does not contain " << quantitiesToRemove; - - scalarQuantities -= quantitiesToRemove; + totals -= quantitiesToRemove; if (resources[slaveId].empty()) { resources.erase(slaveId); @@ -370,27 +352,24 @@ struct RandomSorter::Node const Resources& oldAllocation, const Resources& newAllocation) { - const Resources oldAllocationQuantity = - oldAllocation.createStrippedScalarQuantity(); - const Resources newAllocationQuantity = - newAllocation.createStrippedScalarQuantity(); + const ResourceQuantities oldAllocationQuantities = + ResourceQuantities::fromScalarResources(oldAllocation.scalars()); + const ResourceQuantities newAllocationQuantities = + ResourceQuantities::fromScalarResources(newAllocation.scalars()); CHECK(resources.contains(slaveId)); CHECK(resources[slaveId].contains(oldAllocation)) << "Resources " << resources[slaveId] << " at agent " << slaveId << " does not contain " << oldAllocation; - CHECK(scalarQuantities.contains(oldAllocationQuantity)) - << scalarQuantities << " does not contain " << oldAllocationQuantity; + CHECK(totals.contains(oldAllocationQuantities)) + << totals << " does not contain " << oldAllocationQuantities; resources[slaveId] -= oldAllocation; resources[slaveId] += newAllocation; - scalarQuantities -= oldAllocationQuantity; - scalarQuantities += newAllocationQuantity; - - totals -= ResourceQuantities::fromScalarResources(oldAllocationQuantity); - totals += ResourceQuantities::fromScalarResources(newAllocationQuantity); + totals -= oldAllocationQuantities; + totals += newAllocationQuantities; } // We maintain multiple copies of each shared resource allocated @@ -399,17 +378,10 @@ struct RandomSorter::Node // not been recovered from) a specific client. hashmap<SlaveID, Resources> resources; - // Similarly, we aggregate scalars across slaves and omit information - // about dynamic reservations, persistent volumes and sharedness of - // the corresponding resource. See notes above. - Resources scalarQuantities; - - // To improve the performance of calculating shares, we store - // a redundant but more efficient version of `scalarQuantities`. - // See MESOS-4694. - // - // TODO(bmahler): Can we remove `scalarQuantities` in favor of - // using this type whenever scalar quantities are needed? + // We keep the aggregated scalar resource quantities to speed + // up share calculation. Note, resources shared count are ignored. + // Because sharedness inherently refers to the identities of resources + // and not quantities. ResourceQuantities totals; } allocation; }; diff --git a/src/master/allocator/sorter/sorter.hpp b/src/master/allocator/sorter/sorter.hpp index 25ad48d..d4d039f 100644 --- a/src/master/allocator/sorter/sorter.hpp +++ b/src/master/allocator/sorter/sorter.hpp @@ -109,9 +109,8 @@ public: const std::string& client) const = 0; // Returns the total scalar resource quantities that are allocated to - // this client. This omits metadata about dynamic reservations and - // persistent volumes; see `Resources::createStrippedScalarQuantity`. - virtual const Resources& allocationScalarQuantities( + // this client. + virtual const ResourceQuantities& allocationScalarQuantities( const std::string& client) const = 0; // Returns the clients that have allocations on this slave. @@ -124,10 +123,8 @@ public: const std::string& client, const SlaveID& slaveId) const = 0; - // Returns the total scalar resource quantities in this sorter. This - // omits metadata about dynamic reservations and persistent volumes; see - // `Resources::createStrippedScalarQuantity`. - virtual const Resources& totalScalarQuantities() const = 0; + // Returns the total scalar resource quantities in this sorter. + virtual const ResourceQuantities& totalScalarQuantities() const = 0; // Add resources to the total pool of resources this // Sorter should consider. diff --git a/src/tests/sorter_tests.cpp b/src/tests/sorter_tests.cpp index 1e2791f..76e3256 100644 --- a/src/tests/sorter_tests.cpp +++ b/src/tests/sorter_tests.cpp @@ -1628,15 +1628,17 @@ TYPED_TEST(CommonSorterTest, RemoveSharedResources) sorter.add( slaveId, Resources::parse("cpus:100;mem:100;disk(role1):900").get()); - Resources quantity1 = sorter.totalScalarQuantities(); + ResourceQuantities quantity1 = sorter.totalScalarQuantities(); sorter.add(slaveId, sharedDisk); - Resources quantity2 = sorter.totalScalarQuantities(); + ResourceQuantities quantity2 = sorter.totalScalarQuantities(); - EXPECT_EQ(Resources::parse("disk:100").get(), quantity2 - quantity1); + EXPECT_EQ( + CHECK_NOTERROR(ResourceQuantities::fromString("disk:100")), + quantity2 - quantity1); sorter.add(slaveId, sharedDisk); - Resources quantity3 = sorter.totalScalarQuantities(); + ResourceQuantities quantity3 = sorter.totalScalarQuantities(); EXPECT_NE(quantity1, quantity3); EXPECT_EQ(quantity2, quantity3);
