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 373393bbaaeadf992c2e8d5399462ffe128eaec4 Author: Meng Zhu <[email protected]> AuthorDate: Thu Jun 20 18:48:28 2019 -0700 Removed `setQuota` and `removeQuota` methods in the allocator. These are replaced by the `updateQuota` method. Review: https://reviews.apache.org/r/70918 --- include/mesos/allocator/allocator.hpp | 44 -------------------- src/master/allocator/mesos/allocator.hpp | 38 ----------------- src/master/allocator/mesos/hierarchical.cpp | 63 ----------------------------- src/master/allocator/mesos/hierarchical.hpp | 7 ---- src/master/allocator/mesos/metrics.cpp | 63 +---------------------------- src/master/allocator/mesos/metrics.hpp | 4 -- src/master/quota_handler.cpp | 2 +- src/tests/allocator.hpp | 29 ------------- 8 files changed, 3 insertions(+), 247 deletions(-) diff --git a/include/mesos/allocator/allocator.hpp b/include/mesos/allocator/allocator.hpp index 0aef003..4f9e253 100644 --- a/include/mesos/allocator/allocator.hpp +++ b/include/mesos/allocator/allocator.hpp @@ -414,50 +414,6 @@ public: const std::set<std::string>& roles) = 0; /** - * Informs the allocator to set quota for the given role. - * - * It is up to the allocator implementation how to satisfy quota. An - * implementation may employ different strategies for roles with or - * without quota. Hence an empty (or zero) quota is not necessarily the - * same as an absence of quota. Logically, this method implies that the - * given role should be transitioned to the group of roles with quota - * set. An allocator implementation may assert quota for the given role - * is not set prior to the call and react accordingly if this assumption - * is violated (i.e. fail). - * - * TODO(alexr): Consider returning a future which an allocator can fail - * in order to report failure. - * - * TODO(alexr): Consider adding an `updateQuota()` method which allows - * updating existing quota. - * - * TODO(mzhu): Remove this call in favor of `updateQuota`. - * - */ - virtual void setQuota( - const std::string& role, - const Quota& quota) = 0; - - /** - * Informs the allocator to remove quota for the given role. - * - * An allocator implementation may employ different strategies for roles - * with or without quota. Hence an empty (or zero) quota is not necessarily - * the same as an absence of quota. Logically, this method implies that the - * given role should be transitioned to the group of roles without quota - * set (absence of quota). An allocator implementation may assert quota - * for the given role is set prior to the call and react accordingly if - * this assumption is violated (i.e. fail). - * - * TODO(alexr): Consider returning a future which an allocator can fail in - * order to report failure. - * - * TODO(mzhu): Remove this call in favor of `updateQuota`. - */ - virtual void removeQuota( - const std::string& role) = 0; - - /** * Informs the allocator to update quota for the given role. * * It is up to the allocator implementation how to satisfy quota. An diff --git a/src/master/allocator/mesos/allocator.hpp b/src/master/allocator/mesos/allocator.hpp index 0ae7e36..bedf558 100644 --- a/src/master/allocator/mesos/allocator.hpp +++ b/src/master/allocator/mesos/allocator.hpp @@ -158,13 +158,6 @@ public: const FrameworkID& frameworkId, const std::set<std::string>& roles) override; - void setQuota( - const std::string& role, - const Quota& quota) override; - - void removeQuota( - const std::string& role) override; - void updateQuota( const std::string& role, const Quota2& quota) override; @@ -309,13 +302,6 @@ public: const FrameworkID& frameworkId, const std::set<std::string>& roles) = 0; - virtual void setQuota( - const std::string& role, - const Quota& quota) = 0; - - virtual void removeQuota( - const std::string& role) = 0; - virtual void updateQuota( const std::string& role, const Quota2& quota) = 0; @@ -685,30 +671,6 @@ inline void MesosAllocator<AllocatorProcess>::reviveOffers( template <typename AllocatorProcess> -inline void MesosAllocator<AllocatorProcess>::setQuota( - const std::string& role, - const Quota& quota) -{ - process::dispatch( - process, - &MesosAllocatorProcess::setQuota, - role, - quota); -} - - -template <typename AllocatorProcess> -inline void MesosAllocator<AllocatorProcess>::removeQuota( - const std::string& role) -{ - process::dispatch( - process, - &MesosAllocatorProcess::removeQuota, - role); -} - - -template <typename AllocatorProcess> inline void MesosAllocator<AllocatorProcess>::updateQuota( const std::string& role, const Quota2& quota) diff --git a/src/master/allocator/mesos/hierarchical.cpp b/src/master/allocator/mesos/hierarchical.cpp index c069b24..c05b351 100644 --- a/src/master/allocator/mesos/hierarchical.cpp +++ b/src/master/allocator/mesos/hierarchical.cpp @@ -1397,69 +1397,6 @@ void HierarchicalAllocatorProcess::reviveOffers( } -void HierarchicalAllocatorProcess::setQuota( - const string& role, - const Quota& quota) -{ - CHECK(initialized); - - // This method should be called by the master only if the quota for - // the role is not set. Setting quota differs from updating it because - // the former moves the role to a different allocation group with a - // dedicated sorter, while the later just updates the actual quota. - CHECK(getQuota(role) == DEFAULT_QUOTA); - - // In the legacy `SetQuota` call, quota guarantee also acts as quota limits. - // - // Note, quota has been validated to have no duplicate resource names. - roles[role].quota = Quota2(quota.info); - - metrics.setQuota(role, quota); - - // TODO(alexr): Print all quota info for the role. - LOG(INFO) << "Set quota " << quota.info.guarantee() << " for role '" << role - << "'"; - - // NOTE: Since quota changes do not result in rebalancing of - // offered resources, we do not trigger an allocation here; the - // quota change will be reflected in subsequent allocations. - // - // If we add the ability for quota changes to incur a rebalancing - // of offered resources, then we should trigger that here. -} - - -void HierarchicalAllocatorProcess::removeQuota( - const string& role) -{ - CHECK(initialized); - - // Do not allow removing quota if it is not set. - // Note: it is impossible to set to default quota using the old `setQuota`. - CHECK(getQuota(role) != DEFAULT_QUOTA); - - Role& r = roles.at(role); - - // TODO(alexr): Print all quota info for the role. - LOG(INFO) << "Removed quota " << r.quota.guarantees - << " for role '" << role << "'"; - - r.quota = DEFAULT_QUOTA; - if (r.isEmpty()) { - roles.erase(role); - } - - metrics.removeQuota(role); - - // NOTE: Since quota changes do not result in rebalancing of - // offered resources, we do not trigger an allocation here; the - // quota change will be reflected in subsequent allocations. - // - // If we add the ability for quota changes to incur a rebalancing - // of offered resources, then we should trigger that here. -} - - void HierarchicalAllocatorProcess::updateQuota( const string& role, const Quota2& quota) diff --git a/src/master/allocator/mesos/hierarchical.hpp b/src/master/allocator/mesos/hierarchical.hpp index f14b1fa..8468a88 100644 --- a/src/master/allocator/mesos/hierarchical.hpp +++ b/src/master/allocator/mesos/hierarchical.hpp @@ -425,13 +425,6 @@ public: const FrameworkID& frameworkId, const std::set<std::string>& roles) override; - void setQuota( - const std::string& role, - const Quota& quota) override; - - void removeQuota( - const std::string& role) override; - void updateQuota( const std::string& role, const Quota2& quota) override; diff --git a/src/master/allocator/mesos/metrics.cpp b/src/master/allocator/mesos/metrics.cpp index b61a39b..17ca99e 100644 --- a/src/master/allocator/mesos/metrics.cpp +++ b/src/master/allocator/mesos/metrics.cpp @@ -128,67 +128,6 @@ Metrics::~Metrics() } -void Metrics::setQuota(const string& role, const Quota& quota) -{ - CHECK(!quota_allocated.contains(role)); - - hashmap<string, PullGauge> allocated; - hashmap<string, PullGauge> guarantees; - - foreach (const Resource& resource, quota.info.guarantee()) { - CHECK_EQ(Value::SCALAR, resource.type()); - double value = resource.scalar().value(); - - // TODO(mzhu): use PushGauge for guarantees. - PullGauge guarantee = PullGauge( - "allocator/mesos/quota" - "/roles/" + role + - "/resources/" + resource.name() + - "/guarantee", - process::defer([value]() { return value; })); - - // TODO(mzhu): expose this for every role, including roles - // with default quota. - PullGauge offered_or_allocated( - "allocator/mesos/quota" - "/roles/" + role + - "/resources/" + resource.name() + - "/offered_or_allocated", - defer(allocator, - &HierarchicalAllocatorProcess::_quota_allocated, - role, - resource.name())); - - guarantees.put(resource.name(), guarantee); - allocated.put(resource.name(), offered_or_allocated); - - process::metrics::add(guarantee); - process::metrics::add(offered_or_allocated); - } - - quota_allocated[role] = allocated; - quota_guarantee[role] = guarantees; -} - - -void Metrics::removeQuota(const string& role) -{ - CHECK(quota_allocated.contains(role)); - CHECK(quota_guarantee.contains(role)); - - foreachvalue (const PullGauge& gauge, quota_allocated[role]) { - process::metrics::remove(gauge); - } - - foreachvalue (const PullGauge& gauge, quota_guarantee[role]) { - process::metrics::remove(gauge); - } - - quota_allocated.erase(role); - quota_guarantee.erase(role); -} - - // TODO(mzhu): This currently only updates quota guarantees. // Add metrics for quota limits as well. void Metrics::updateQuota(const string& role, const Quota2& quota) @@ -196,6 +135,8 @@ void Metrics::updateQuota(const string& role, const Quota2& quota) // This is the "remove" case where the role's quota // is set to the default. if (quota.guarantees == DEFAULT_QUOTA.guarantees) { + // TODO(mzhu): always expose the allocated gauge event if + // a role only has default quota. foreachvalue (const PullGauge& gauge, quota_allocated[role]) { process::metrics::remove(gauge); } diff --git a/src/master/allocator/mesos/metrics.hpp b/src/master/allocator/mesos/metrics.hpp index a0953b0..e9174a1 100644 --- a/src/master/allocator/mesos/metrics.hpp +++ b/src/master/allocator/mesos/metrics.hpp @@ -48,10 +48,6 @@ struct Metrics ~Metrics(); - // TODO(mzhu): Remove these in favor of `updateQuota`. - void setQuota(const std::string& role, const Quota& quota); - void removeQuota(const std::string& role); - void updateQuota(const std::string& role, const Quota2& quota); void addRole(const std::string& role); diff --git a/src/master/quota_handler.cpp b/src/master/quota_handler.cpp index 0180b27..b5d2965 100644 --- a/src/master/quota_handler.cpp +++ b/src/master/quota_handler.cpp @@ -669,7 +669,7 @@ Future<http::Response> Master::QuotaHandler::__set( // Rescind outstanding offers to facilitate satisfying the quota request. // NOTE: We set quota before we rescind to avoid a race. If we were to // rescind first, then recovered resources may get allocated again - // before our call to `setQuota` was handled. + // before our call to `updateQuota` was handled. // The consequence of setting quota first is that (in the hierarchical // allocator) it will trigger an allocation. This means the rescinded // offer resources will only be available to quota once another diff --git a/src/tests/allocator.hpp b/src/tests/allocator.hpp index 45e415b..1ca85c5 100644 --- a/src/tests/allocator.hpp +++ b/src/tests/allocator.hpp @@ -200,18 +200,6 @@ ACTION_P(InvokeReviveOffers, allocator) } -ACTION_P(InvokeSetQuota, allocator) -{ - allocator->real->setQuota(arg0, arg1); -} - - -ACTION_P(InvokeRemoveQuota, allocator) -{ - allocator->real->removeQuota(arg0); -} - - ACTION_P(InvokeUpdateQuota, allocator) { allocator->real->updateQuota(arg0, arg1); @@ -378,16 +366,6 @@ public: EXPECT_CALL(*this, reviveOffers(_, _)) .WillRepeatedly(DoDefault()); - ON_CALL(*this, setQuota(_, _)) - .WillByDefault(InvokeSetQuota(this)); - EXPECT_CALL(*this, setQuota(_, _)) - .WillRepeatedly(DoDefault()); - - ON_CALL(*this, removeQuota(_)) - .WillByDefault(InvokeRemoveQuota(this)); - EXPECT_CALL(*this, removeQuota(_)) - .WillRepeatedly(DoDefault()); - ON_CALL(*this, updateQuota(_, _)) .WillByDefault(InvokeUpdateQuota(this)); EXPECT_CALL(*this, updateQuota(_, _)) @@ -520,13 +498,6 @@ public: const FrameworkID&, const std::set<std::string>&)); - MOCK_METHOD2(setQuota, void( - const std::string&, - const Quota&)); - - MOCK_METHOD1(removeQuota, void( - const std::string&)); - MOCK_METHOD2(updateQuota, void( const std::string&, const Quota2&));
