Refactored operators for Resource object and made them private. Review: https://reviews.apache.org/r/28089
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/a01773b5 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/a01773b5 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/a01773b5 Branch: refs/heads/master Commit: a01773b5fae7a434d508e04217aa3285e71fdc0a Parents: 05b5ffa Author: Jie Yu <[email protected]> Authored: Fri Nov 14 14:20:45 2014 -0800 Committer: Jie Yu <[email protected]> Committed: Wed Nov 19 00:14:25 2014 -0800 ---------------------------------------------------------------------- include/mesos/resources.hpp | 23 ++---- src/common/resources.cpp | 162 +++++++++++++++++++------------------ src/tests/resources_tests.cpp | 8 +- 3 files changed, 95 insertions(+), 98 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/a01773b5/include/mesos/resources.hpp ---------------------------------------------------------------------- diff --git a/include/mesos/resources.hpp b/include/mesos/resources.hpp index 4ed8cee..c8a261f 100644 --- a/include/mesos/resources.hpp +++ b/include/mesos/resources.hpp @@ -43,23 +43,6 @@ namespace mesos { -bool operator == (const Resource& left, const Resource& right); -bool operator != (const Resource& left, const Resource& right); - - -bool operator <= (const Resource& left, const Resource& right); - - -Resource& operator += (Resource& left, const Resource& right); -Resource operator + (const Resource& left, const Resource& right); -Resource& operator -= (Resource& left, const Resource& right); -Resource operator - (const Resource& left, const Resource& right); - - -// Return true iff both Resources have the same name, type, and role. -bool matches(const Resource& left, const Resource& right); - - // TODO(bmahler): Ensure that the underlying resources are kept // in a flattened state: MESOS-1714. class Resources @@ -98,6 +81,12 @@ public: Resources() {} + // TODO(jieyu): Consider using C++11 initializer list. + /*implicit*/ Resources(const Resource& resource) + { + resources.Add()->CopyFrom(resource); + } + /*implicit*/ Resources(const google::protobuf::RepeatedPtrField<Resource>& _resources) { http://git-wip-us.apache.org/repos/asf/mesos/blob/a01773b5/src/common/resources.cpp ---------------------------------------------------------------------- diff --git a/src/common/resources.cpp b/src/common/resources.cpp index 810b698..458c1cd 100644 --- a/src/common/resources.cpp +++ b/src/common/resources.cpp @@ -35,19 +35,75 @@ using std::vector; namespace mesos { +///////////////////////////////////////////////// +// Helper functions. +///////////////////////////////////////////////// + +// Tests if we can add two Resource objects together resulting in one +// valid Resource object. For example, two Resource objects with +// different name, type or role are not addable. +static bool addable(const Resource& left, const Resource& right) +{ + return left.name() == right.name() && + left.type() == right.type() && + left.role() == right.role(); +} + + +// Tests if we can subtract "right" from "left" resulting in one valid +// Resource object. For example, two Resource objects with different +// name, type or role are not subtractable. +// NOTE: Set substraction is always well defined, it does not require +// 'right' to be contained within 'left'. For example, assuming that +// "left = {1, 2}" and "right = {2, 3}", "left" and "right" are +// subtractable because "left - right = {1}". However, "left" does not +// contains "right". +static bool subtractable(const Resource& left, const Resource& right) +{ + return left.name() == right.name() && + left.type() == right.type() && + left.role() == right.role(); +} + + +// Tests if "right" is contained in "left". +static bool contains(const Resource& left, const Resource& right) +{ + if (left.name() != right.name() || + left.type() != right.type() || + left.role() != right.role()) { + return false; + } + + if (left.type() == Value::SCALAR) { + return right.scalar() <= left.scalar(); + } else if (left.type() == Value::RANGES) { + return right.ranges() <= left.ranges(); + } else if (left.type() == Value::SET) { + return right.set() <= left.set(); + } else { + return false; + } +} + + bool operator == (const Resource& left, const Resource& right) { - if (matches(left, right)) { - if (left.type() == Value::SCALAR) { - return left.scalar() == right.scalar(); - } else if (left.type() == Value::RANGES) { - return left.ranges() == right.ranges(); - } else if (left.type() == Value::SET) { - return left.set() == right.set(); - } + if (left.name() != right.name() || + left.type() != right.type() || + left.role() != right.role()) { + return false; } - return false; + if (left.type() == Value::SCALAR) { + return left.scalar() == right.scalar(); + } else if (left.type() == Value::RANGES) { + return left.ranges() == right.ranges(); + } else if (left.type() == Value::SET) { + return left.set() == right.set(); + } else { + return false; + } } @@ -59,32 +115,19 @@ bool operator != (const Resource& left, const Resource& right) bool operator <= (const Resource& left, const Resource& right) { - if (matches(left, right)) { - if (left.type() == Value::SCALAR) { - return left.scalar() <= right.scalar(); - } else if (left.type() == Value::RANGES) { - return left.ranges() <= right.ranges(); - } else if (left.type() == Value::SET) { - return left.set() <= right.set(); - } - } - - return false; + return contains(right, left); } Resource& operator += (Resource& left, const Resource& right) { - if (matches(left, right)) { - if (left.type() == Value::SCALAR) { - left.mutable_scalar()->MergeFrom(left.scalar() + right.scalar()); - } else if (left.type() == Value::RANGES) { - left.mutable_ranges()->Clear(); - left.mutable_ranges()->MergeFrom(left.ranges() + right.ranges()); - } else if (left.type() == Value::SET) { - left.mutable_set()->Clear(); - left.mutable_set()->MergeFrom(left.set() + right.set()); - } + // TODO(jieyu): Leverage += for Value to avoid copying. + if (left.type() == Value::SCALAR) { + left.mutable_scalar()->CopyFrom(left.scalar() + right.scalar()); + } else if (left.type() == Value::RANGES) { + left.mutable_ranges()->CopyFrom(left.ranges() + right.ranges()); + } else if (left.type() == Value::SET) { + left.mutable_set()->CopyFrom(left.set() + right.set()); } return left; @@ -94,35 +137,20 @@ Resource& operator += (Resource& left, const Resource& right) Resource operator + (const Resource& left, const Resource& right) { Resource result = left; - - if (matches(left, right)) { - if (left.type() == Value::SCALAR) { - result.mutable_scalar()->MergeFrom(left.scalar() + right.scalar()); - } else if (left.type() == Value::RANGES) { - result.mutable_ranges()->Clear(); - result.mutable_ranges()->MergeFrom(left.ranges() + right.ranges()); - } else if (left.type() == Value::SET) { - result.mutable_set()->Clear(); - result.mutable_set()->MergeFrom(left.set() + right.set()); - } - } - + result += right; return result; } Resource& operator -= (Resource& left, const Resource& right) { - if (matches(left, right)) { - if (left.type() == Value::SCALAR) { - left.mutable_scalar()->MergeFrom(left.scalar() - right.scalar()); - } else if (left.type() == Value::RANGES) { - left.mutable_ranges()->Clear(); - left.mutable_ranges()->MergeFrom(left.ranges() - right.ranges()); - } else if (left.type() == Value::SET) { - left.mutable_set()->Clear(); - left.mutable_set()->MergeFrom(left.set() - right.set()); - } + // TODO(jieyu): Leverage -= for Value to avoid copying. + if (left.type() == Value::SCALAR) { + left.mutable_scalar()->CopyFrom(left.scalar() - right.scalar()); + } else if (left.type() == Value::RANGES) { + left.mutable_ranges()->CopyFrom(left.ranges() - right.ranges()); + } else if (left.type() == Value::SET) { + left.mutable_set()->CopyFrom(left.set() - right.set()); } return left; @@ -132,31 +160,11 @@ Resource& operator -= (Resource& left, const Resource& right) Resource operator - (const Resource& left, const Resource& right) { Resource result = left; - - if (matches(left, right)) { - if (left.type() == Value::SCALAR) { - result.mutable_scalar()->MergeFrom(left.scalar() - right.scalar()); - } else if (left.type() == Value::RANGES) { - result.mutable_ranges()->Clear(); - result.mutable_ranges()->MergeFrom(left.ranges() - right.ranges()); - } else if (left.type() == Value::SET) { - result.mutable_set()->Clear(); - result.mutable_set()->MergeFrom(left.set() - right.set()); - } - } - + result -= right; return result; } -bool matches(const Resource& left, const Resource& right) -{ - return left.name() == right.name() && - left.type() == right.type() && - left.role() == right.role(); -} - - ///////////////////////////////////////////////// // Public static functions. ///////////////////////////////////////////////// @@ -433,7 +441,7 @@ Option<Resources> Resources::find( Option<Resource> Resources::get(const Resource& r) const { foreach (const Resource& resource, resources) { - if (matches(resource, r)) { + if (addable(resource, r)) { return resource; } } @@ -745,7 +753,7 @@ Resources Resources::operator + (const Resource& that) const bool added = false; foreach (const Resource& resource, resources) { - if (matches(resource, that)) { + if (addable(resource, that)) { result.resources.Add()->MergeFrom(resource + that); added = true; } else { @@ -795,7 +803,7 @@ Resources Resources::operator - (const Resource& that) const Resources result; foreach (const Resource& resource, resources) { - if (matches(resource, that)) { + if (subtractable(resource, that)) { Resource r = resource - that; if (!isZero(r)) { result.resources.Add()->MergeFrom(r); http://git-wip-us.apache.org/repos/asf/mesos/blob/a01773b5/src/tests/resources_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/resources_tests.cpp b/src/tests/resources_tests.cpp index 9fe3c3a..c8d069a 100644 --- a/src/tests/resources_tests.cpp +++ b/src/tests/resources_tests.cpp @@ -248,8 +248,8 @@ TEST(ResourcesTest, ScalarEquals) EXPECT_EQ(2u, r2.size()); EXPECT_EQ(r1, r2); - Resource cpus1 = Resources::parse("cpus", "3", "role1").get(); - Resource cpus2 = Resources::parse("cpus", "3", "role2").get(); + Resources cpus1 = Resources::parse("cpus", "3", "role1").get(); + Resources cpus2 = Resources::parse("cpus", "3", "role2").get(); EXPECT_NE(cpus1, cpus2); } @@ -287,8 +287,8 @@ TEST(ResourcesTest, ScalarSubset2) Resources r2; r2 += cpus2; - EXPECT_FALSE(cpus1 <= cpus2); - EXPECT_FALSE(cpus2 <= cpus1); + EXPECT_FALSE(r1 <= r2); + EXPECT_FALSE(r2 <= r1); Resource cpus3 = Resources::parse("cpus", "3", "role1").get();
