----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29742/ -----------------------------------------------------------
(Updated Feb. 11, 2015, 11:23 p.m.) Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, and Vinod Kone. Changes ------- Updated description to clearly define the point of this patch. Repository: mesos Description (updated) ------- # 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 of the code as well as 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
