mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Park" <mcyp...@gmail.com>
Subject Re: Review Request 30654: Update the filter abstraction for Resources.
Date Thu, 12 Feb 2015 00:47:40 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message