> On Feb. 13, 2015, 12:35 p.m., Alexander Rukletsov wrote: > > src/master/validation.cpp, line 105 > > <https://reviews.apache.org/r/29742/diff/6/?file=860711#file860711line105> > > > > `!Resources::persistentVolume(resource)`, if my mental boolean algebra > > works. Could you please grep the code base for these De Morgan cases? > > > > Also, this is a good case why predicates are beneficial and must be > > favoured => move this example into JIRA as an argument and for the record.
Good catch! I even mentioned this case in the **Engineering Benefits** section and missed it myself. Embarrassing, Haha. > On Feb. 13, 2015, 12:35 p.m., Alexander Rukletsov wrote: > > include/mesos/resources.hpp, line 92 > > <https://reviews.apache.org/r/29742/diff/6/?file=860708#file860708line92> > > > > I personally prefer naming predicates `is<...>` for clarity, but it > > seems we don't have consensus here, feel free to ignore. I think Vinod cares about consistency and Jie and I actually prefer the `is` prefix as well. I'll be making this change shortly. - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29742/#review72358 ----------------------------------------------------------- On Feb. 11, 2015, 11:49 p.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29742/ > ----------------------------------------------------------- > > (Updated Feb. 11, 2015, 11:49 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 its 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 f39a876cdd6b580a7a75fd053e6923761bee7635 > > Diff: https://reviews.apache.org/r/29742/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Michael Park > >
