----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29736/#review71155 -----------------------------------------------------------
Patch looks great! Reviews applied: [28697, 29742, 29736] All tests passed. - Mesos ReviewBot On Feb. 5, 2015, 1:40 a.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29736/ > ----------------------------------------------------------- > > (Updated Feb. 5, 2015, 1:40 a.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, and > Vinod Kone. > > > Repository: mesos > > > Description > ------- > > 1. A `Resources::Filter` constructs off of a predicate and the operator () > has a single implementation: > > ```cpp > Resources result; > foreach (const Resource& resource, resources) { > if (pred(resource)) { > result += resource; > } > } > return result; > ``` > > Advantages of this approach: > a. No new class definition is necessary to create a new filter. > - We can use static functions to create a new filter inline. e.g. > Filter(persistentVolume) or Filter(empty) > - We can use lambdas (later on) to create a new filter inline. > Filter([](const Resource& resource) { ... }) > - We don't need to write the same for loop with different predicates > for every new filter. > b. The resulting set of resources are guaranteed to consist only of > resources that existed in the original resources. > - The only guarantee we enforce with a derived filter class that > inherits from a base filter class is > that we take Resources and return Resources. Nothing enforces that > the resulting resources must only be > consisted of existing resources. > > 2. We can naturally compose filters with logical operators. > > Suppose we want a filter that returns resources that are reserved for a > particular `ReservationType`, and > a filter that returns persistent volumes. It may look something like this in > the old approach. > > ```cpp > // Returns resources that are reserved for a particular ReservationType. > struct ReservedFilter : Filter > { > explicit ReservedFilter(const Resource::ReservationType& _reservationType) > : reservationType(_reservationType) {} > > virtual Resources apply(const Resources& resources) const { > Resources result; > foreach (const Resource& resource, resources) { > if (isReserved(resource, reservationType)) { > result += resource; > } > } > return result; > } > > Resource::ReservationType reservationType; > } > > // Returns persistent volumes. > struct PersistentVolumeFilter : Filter > { > virtual Resources apply(const Resources& resources) const { > Resources result; > foreach (const Resource& resource, resources) { > if (isPersistentVolume(resource)) { > result += resource; > } > } > return result; > } > } > ``` > > and we can use them like so: > > ```cpp > ReservedFilter(Resource::DYNAMIC).apply(resources); > PersistentVolumeFilter().apply(resources); > ``` > > But how about when we want to now create a filter that returns resources that > are persistent volumes **and** > reserved for a particular `ReservationType`? Ideally we would compose these 2 > existing filters somehow, > but we end up having to duplicate most of the code from each filter and write > a new filter definition > (or perform both filters and intersect the results, yikes.): > > ```cpp > // Returns persistent volumes that are reserved for a particular > ReservationType. > struct ReservedPersistentVolumeFilter : Filter > { > explicit ReservedFilter(const Resource::ReservationType& _reservationType) > : reservationType(_reservationType) {} > > virtual Resources apply(const Resources& resources) const { > Resources result; > foreach (const Resource& resource, resources) { > if (reserved(resource, reservationType) && persistentVolume(resource)) { > result += resource; > } > } > return result; > } > > Resource::ReservationType reservationType; > } > ``` > > and it would be used like so: > > ```cpp > ReservedPersistentVolumeFilter(Resource::DYNAMIC).apply(resources); > ``` > > Here's what the above looks like in the new design. > > ```cpp > class Filter > { > public: > > // Useful predefined filters. > static Filter reserved(const Resource::ReservationType& reservationType) > { > return Filter(lambda::bind(Resources::reserved, lambda::_1, > reservationType)); > } > > static Filter persistentVolume() > { > return Filter(Resources::persistentVolume); > } > > /* Generic Filter definition */ > > }; > ``` > > These can be used like so: > > ```cpp > Filter::reserved(Resource::DYNAMIC)(resources); > Filter::persistentVolume()(resources); > ``` > > With logical operators, we can compose them inline like so: > > ```cpp > Filter filter = > Filter::reserved(Resource::DYNAMIC) && Filter::persistentVolume(); > Resources filtered = filter(resources); > ``` > > If this composition is common, then we can always add another static function > under `Filter`: > > ```cpp > class Filter > { > static Filter reserved(const Resource::ReservationType& reservationType); > static Filter persistentVolume(); > > static Filter reservedPersistentVolume(const Resource::ReservationType& > reservationType) > { > return Filter::reserved(Resource::DYNAMIC) && Filter::persistentVolume(); > } > > /* ... */ > }; > ``` > > At which point we can use it like so: > > ```cpp > Filter::reservedPersistentVolume(Resource::DYNAMIC)(resources); > ``` > > > Diffs > ----- > > include/mesos/resources.hpp 3b57568c10233a0c692787de6464f21af5eaadf4 > src/common/resources.cpp 308b8860fec3db5bfab3925423afda037bfbc4e7 > src/master/hierarchical_allocator_process.hpp > 6b4489276d9f72f3bd99066bf7e48dba5ebe537e > src/tests/hierarchical_allocator_tests.cpp > f44d9e98d6d9db9621f5361cddb6134f90277180 > src/tests/resources_tests.cpp 4744e872b082553046ecc0e344403754ee685842 > > Diff: https://reviews.apache.org/r/29736/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Michael Park > >
