----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29742/ -----------------------------------------------------------
(Updated Feb. 13, 2015, 10:58 p.m.) Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, and Vinod Kone. Changes ------- Updated the functions to be prefixed with `is`. Found a few De Morgan cases for `isPersistentVolume`. 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 (updated) ----- 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
