----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29742/#review72358 -----------------------------------------------------------
Nice motivation for the change, Michael! Let's create a JIRA for this change and move/copy the motivation there. Also, +1 for engineering benefits, we should definitely adhere to ODR when we write code. include/mesos/resources.hpp <https://reviews.apache.org/r/29742/#comment118440> I personally prefer naming predicates `is<...>` for clarity, but it seems we don't have consensus here, feel free to ignore. src/master/validation.cpp <https://reviews.apache.org/r/29742/#comment118441> Does it makes sense to use `Resources::persistentVolume()` predicate here? IIUC we do want check exactly this, but split the conditions between several if clauses. However, if in the future the definition of `Resources::persistentVolume()` will change, we will still capture the right intention. Maybe we can even conflate these two if clauses in one, if my mental boolean logic works right : ). The disadvantage is however that it becomes not obvious that we skip resources without disk, and therefore this should be documented in a comment. ``` // Skip resources that don't have disk. foreach (const Resource& resource, resources) { if (Resources::persistentVolume(resource)) { ... } else if (resource.has_disk() && resource.disk().has_volume()) { return Error("Non-persistent volume not supported"); } else if (resource.has_disk()) { return Error("DiskInfo is set but empty"); } } ``` src/master/validation.cpp <https://reviews.apache.org/r/29742/#comment118442> `!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. - Alexander Rukletsov 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 > >
