----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29742/#review72659 -----------------------------------------------------------
Ship it! Thanks for the cleanup! src/master/validation.cpp <https://reviews.apache.org/r/29742/#comment118712> The indent here should be 4 spaces. - Jie Yu On Feb. 13, 2015, 10:59 p.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29742/ > ----------------------------------------------------------- > > (Updated Feb. 13, 2015, 10:59 p.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, and > Vinod Kone. > > > Repository: mesos > > > Description > ------- > > # Motivation > > The main motivation for introducing these functions is to capture the > definition of various identification of resources. With these functions > capturing various definitions of concepts for us, we gain: > - readability. > - engineering benefits. > > ## Example > > For example, consider the concept of "persistent volume". Currently we do `if > (resource.has_disk() && resource.has_persistence())` throughout the codebase > to test to identify this type of resource. > > ### Readability > > From a readability perspective, `if (resource.has_disk() && > resource.has_persistence())` simply harder to read than `if > (Resource::persistentVolume(resource))`. A foreign reader also can't be sure > that the first predicate is checking for a persistent volume without digging > deeper into the codebase. (Maybe we actually have an additional requirement > for a resource to be considered a persistent volume.) > > ### Engineering Benefit > > If and when we realize that the definition needs to be updated, we shouldn't > have to change the predicate every `if` statement that checks for a > persistent volume. > > If you're thinking, "just grep for `if (resource.has_disk() && > resource.has_persistence())`...", what if we didn't use `resource` as the > variable name? what if we actually did `if (!(resource.has_disk() && > resource.has_persistence()))`? what about `if (!resource.has_disk() || > !resource.has_persistence()))`? In general I believe this approach makes it > hard to keep the definitions consistent throughout the codebase. > > Instead, we should consistently use the predicates that capture the > definition, (e.g. `Resource::persistentVolume(resource)`) and later on if we > change the definition of "persistent volume", we simply update the definition > of `persistentVolume` and we're done. > > ## Why not just have a Filter instead? > > Fundamentally a `Filter` is built on a **unary predicate**. Given a list of > elements, we keep elements that satisfy the predicate. We *could* embed these > predicates into a `Filter` and only provide those. But 1. I don't think a > `Filter` is necessarily the right tool for every job. 2. Unary predicates are > the basis of many algorithms (e.g. `all_of`, `any_of`, `none_of`, `count_if`, > `find_if`) and therefore deserve to exist in their own right. > > > Diffs > ----- > > include/mesos/resources.hpp c7cc46e0183ea97013dd088a717da6c0e6ed5cf0 > src/common/resources.cpp 98371f6873482d0cdbefeb279b58ae6cc680a88f > src/master/master.hpp 6a39df04514c756415354fae66c5835ada191c52 > src/master/validation.cpp acc35b25c93f2d3900d79c3070b1d681416ed66b > src/slave/slave.cpp ec7ec1356e745bb07484ae1755c9183b038043b3 > > Diff: https://reviews.apache.org/r/29742/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Michael Park > >
