> On Feb. 3, 2015, 8:19 p.m., Vinod Kone wrote: > > include/mesos/resources.hpp, line 92 > > <https://reviews.apache.org/r/29742/diff/4/?file=839661#file839661line92> > > > > We didn't do "isempty" above, so how about getting rid of "is" as > > prefix? I think returning a "bool" signals a "is". > > Michael Park wrote: > Hm, that's a good point. I'll fix that. > > Michael Park wrote: > Fixed.
Hum, actually, I prefer the name 'isEmpty', 'isPersistentVolume', 'isReserved' because there are 'reserved' and 'unreserved' functions (same name) in Resources which return reserved/unreserved resources (different purpose). - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29742/#review70817 ----------------------------------------------------------- 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 > >
