-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30654/
-----------------------------------------------------------
(Updated Feb. 12, 2015, 12:47 a.m.)
Review request for mesos, Ben Mahler and Jie Yu.
Changes
-------
Updated description to clearly define the objective of this patch.
Summary (updated)
-----------------
Update the filter abstraction for Resources.
Repository: mesos
Description (updated)
-------
# Motivation
The main motivation here is to improve the design of the filter abstraction for
`Resources`. With the new design we gain:
1. No duplicate code.
2. No need to introduce a new class for every filter.
3. Safer code with less work.
4. Ability to compose filters.
## Overview of the Current Design.
I think I'll need to start here to provide a clear motivation. Here's the
current design in short:
```cpp
class Filter {
public:
virtual Resources apply(const Resources& resources) const = 0;
};
class FooFilter : public Filter {
public:
virtual Resources apply(const Resource& resources) const {
Resources result;
foreach (const Resource& resource, resources) {
if (/* resource is Foo. */) {
result += resource;
}
}
return result;
}
};
class BarFilter : public Filter {
public:
virtual Resources apply(const Resource& resources) const {
Resources result;
foreach (const Resource& resource, resources) {
if (/* resource is Bar. */) {
result += resource;
}
}
return result;
}
};
```
### Disadvantages
1. Duplicate code.
Every derived `Filter` will have duplicate code, in specific:
```cpp
Resources result;
foreach (const Resource& resource, resources) {
if (/* resource satisfies some predicate. */) {
result += resource;
}
}
return result;
```
2. Need to introduce a new class definition for every new `Filter`.
We should be able to create new filters inline and use them in cases where the
filter is only needed once and would only hurt readability at the global level.
If the filter is useful in many contexts, by all means give it a name and put
it somewhere.
This is equivalent to lambda expressions which allow us to create new functions
inline in cases where the function is only useful in this specific context but
would only hurt readability at the global level. If the lambda is useful in
many contexts, we give it a name and put it somewhere.
3. The constraints are too weak.
A `Filter` must return a subset of the original `Resources`. It need not be a
strict subset, but it must not be a strict superset. With the pure virtual
apply, the only constraint we put on a new `Filter` definition is that we take
`Resources` and return `Resources`. We should strive for code that prevents
preventable bugs.
4. Inability to compose filters.
I've defined 2 filters above, `FooFilter` and `BarFilter`. We should be able to
give rise to a new filter with a composition of the filters above. For example,
if I wanted to `AND` the filters, I shouldn't have to introduce
`FooAndBarFilter`.
This is equivalent to if we had predicates for these. Suppose we have
`isFoo(resource)` and `isBar(resource)`. Would we introduce `isNotFoo`,
`isNotBar`, `isFooAndBar`, `isFooOrBar`, `isFooNotBar`, etc? If `FooAndBar` is
a common concept we use all the time, sure. But in general, we would simply
compose our predicates: `!isFoo(resource)`, `!isBar(resource)`,
`isFoo(resource) && isBar(resource)`, `isFoo(resource) || isBar(resource)`,
`isFoo(resource) && !isBar(resource)`.
## Overview of the New Design
```cpp
class Filter {
public:
typedef lambda::function<bool(const Resource&)> Predicate;
Filter(const Predicate& _predicate)
: predicate(_predicate) {}
Resources operator()(const Resources& resources) const {
Resources result;
foreach (const Resource& resource, resources) {
if (predicate(resource)) {
result += resource;
}
}
return result;
}
Filter operator ! ();
private:
Filter operator && (
const Filter &lhs,
const Filter &rhs);
Filter operator || (
const Filter &lhs,
const Filter &rhs);
Predicate predicate;
};
bool isFoo(const Resource& resource) {
return /* resource is Foo. */
}
Filter FooFilter = Filter(isFoo);
Filter BarFilter = Filter([](const Resource& resource) {
return /* resource is Bar. */
});
```
### Addressing the Disadvantages
1. No duplicate code.
We've removed the duplicate code by making the predicate the customization
point.
2. No need to introduce a new class definition.
We can certainly introduce named filters if they're useful in many contexts
such as `FooFilter` and `BarFilter` above, but there's no **need** to.
3. The constraints are strong.
The creator of a new `Filter` simply specifies the predicate. The constraint,
"filter returns a subset of the original Resources" cannot be violated.
4. Composability.
Using the overloaded logical operators (We can change this to be named
functions if people prefer), we can compose existing filters.
`Filter FooAndBarFilter = FooFilter && BarFilter;` gives us a composed filter
that requires both filters to be satisfied.
Diffs
-----
include/mesos/resources.hpp c7cc46e0183ea97013dd088a717da6c0e6ed5cf0
src/common/resources.cpp 98371f6873482d0cdbefeb279b58ae6cc680a88f
src/master/hierarchical_allocator_process.hpp
10fa6ec4116174f0fa597714976507a6c54f082b
src/master/master.hpp 6a39df04514c756415354fae66c5835ada191c52
src/tests/hierarchical_allocator_tests.cpp
df844b57df5f9bb886e2e380d8751c809d3fbd5b
src/tests/resources_tests.cpp 3f98782fd437dba808d720bf8e9b94b8fa7e0feb
Diff: https://reviews.apache.org/r/30654/diff/
Testing (updated)
-------
make check
Thanks,
Michael Park
