[jira] [Comment Edited] (MESOS-2348) Introduce a new filter abstraction for Resources.

["Michael Park (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/MESOS-2348?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14319380#comment-14319380 ] 

Michael Park edited comment on MESOS-2348 at 2/13/15 1:27 AM:
--------------------------------------------------------------

Hey Alex, thanks for the feedback!

I would agree with this approach if this statement were true: "a general filter may be something more than just a map over resources with the given predicate". A filter by definition is:

{quote}
a higher-order function that processes a data structure (typically a list) in some order to produce a new data structure containing exactly those elements of the original data structure for which a given predicate returns the boolean value true.
{quote}

**Source**: http://en.wikipedia.org/wiki/Filter_%28higher-order_function%29

So what's going on with {{RoleFilter}}? This is a **great** example of how the current design can lead people to believe that there are mysterious filters that are somehow more generic than the "predicate filter". The {{RoleFilter}} is actually 3 filters conflated into one, and simply dispatches to different filters based on the given arguments.

Here is the mapping:
  1. {{RoleFilter("R")}} => {{Resources::Filter::reserved("R")}}
  2. {{RoleFilter("*")}} => {{Resources::Filter::unreserved()}}
  3. {{RoleFilter()}} => {{Resources::Filter::any()}}

The current use of {{RoleFilter}} is when we want an {{initializer_list}} of them.

{code}
std::initializer_list<RoleFilter> filters = {
  RoleFilter(role),
  RoleFilter("*"),
  RoleFilter()
};
{code}

This is directly translated to:

{code}
std::initializer_list<Filter> filters = {
  Resources::Filter::reserved(role),
  Resources::Filter::unreserved(),
  Resources::Filter::any()
};
{code}

If we actually wanted a unified interface for {{RoleFilter}}, we can also provide that

{code}
class Resources {
  public:

  class Filter {
    public:
    static Filter any();
    static Filter reserved(const std::string &role);
    static Filter unreserved();

    static Filter role() {
      return any();
    }

    static Filter role(const std::string &role) {
      return role == "*" ? unreserved() : reserved(role);
    }

  };
};
{code}

This is now the unified interface that dispatches to different filters:

{code}
std::initializer_list<Filter> filters = {
  Resources::Filter::role(role),
  Resources::Filter::role("*"),
  Resources::Filter::role()
};
{code}


was (Author: mcypark):
Hey Alex, thanks for the feedback!

I would agree with this approach if this statement were true: "a general filter may be something more than just a map over resources with the given predicate". A filter by definition is:

{quote}
a higher-order function that processes a data structure (typically a list) in some order to produce a new data structure containing exactly those elements of the original data structure for which a given predicate returns the boolean value true.
{quote}

**Source**: http://en.wikipedia.org/wiki/Filter_%28higher-order_function%29

So what's going on with {{RoleFilter}}? This is a **great** example of how the current design can lead people to believe that there are mysterious filters that are somehow more generic than the "predicate filter". The {{RoleFilter}} is actually 3 filters conflated into one, and simply dispatches to different filters based on the given arguments.

Here is the mapping:
  1. {{RoleFilter("R")}} => {{Resources::Filter::reserved("R")}}
  2. {{RoleFilter("*")}} => {{Resources::Filter::unreserved()}}
  3. {{RoleFilter()}} => {{Resources::Filter::any()}}

The current use of {{RoleFilter}} is when we want an {{initializer_list}} of them.

<code>
std::initializer_list<RoleFilter> filters = {
  RoleFilter(role),
  RoleFilter("*"),
  RoleFilter()
};
<code>

This is directly translated to:

<code>
std::initializer_list<Filter> filters = {
  Resources::Filter::reserved(role),
  Resources::Filter::unreserved(),
  Resources::Filter::any()
};
<code>

If we actually wanted a unified interface for {{RoleFilter}}, we can also provide that

<code>
class Resources {
  public:

  class Filter {
    public:
    static Filter any();
    static Filter reserved(const std::string &role);
    static Filter unreserved();

    static Filter role() {
      return any();
    }

    static Filter role(const std::string &role) {
      return role == "*" ? unreserved() : reserved(role);
    }

  };
};
<code>

This is now the unified interface that dispatches to different filters:

<code>
std::initializer_list<Filter> filters = {
  Resources::Filter::role(role),
  Resources::Filter::role("*"),
  Resources::Filter::role()
};
<code>

> Introduce a new filter abstraction for Resources.
> -------------------------------------------------
>
>                 Key: MESOS-2348
>                 URL: https://issues.apache.org/jira/browse/MESOS-2348
>             Project: Mesos
>          Issue Type: Task
>          Components: general
>            Reporter: Michael Park
>            Assignee: Michael Park
>              Labels: mesosphere
>
> h1. 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.
> h2. 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:
> {code}
> 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;
>   }
> };
> {code}
> h3. Disadvantages
> 1. Duplicate code.
> Every derived {{Filter}} will have duplicate code, in specific:
> {code}
> Resources result;
> foreach (const Resource& resource, resources) {
>   if (/* resource satisfies some predicate. */) {
>     result += resource;
>   }
> }
> return result;
> {code}
> 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)}}.
> h2. Overview of the New Design
> {code}
> 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. */
> });
> {code}
> h3. 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.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)