> On Feb. 13, 2015, 12:35 p.m., Alexander Rukletsov wrote: > > src/master/validation.cpp, line 65 > > <https://reviews.apache.org/r/29742/diff/6/?file=860711#file860711line65> > > > > 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"); > > } > > } > > ``` > > Michael Park wrote: > Your boolean algebra seemed to have worked right, but I'm a bit wary of > this case. I think these functions should assume valid input, which leads me > to think that they shouldn't be used in validation code. For example, using > `isPersistentVolume` in a function that validates DiskInfo which contains > persistent volume I'm not sure is a good idea. > > It seems that specific `if` statement is meant to catch persistent > volumes, and you're right that if the definition of persistent volumes > change, we'll still capture the right intention. The caveat here I think is > that we capture the right intention for **that** `if` statement, but it's not > obvious which cases we still need to cover. > > Let's consider another example. With ReservationType introduced, we have > 3 valid states and 1 invalid state for a `(role, reservation_type)` pair. > (Please ignore the exact terminology here as that's planned to change) > > 1. ("*", STATIC) -- unreserved > 2. ("*", DYNAMIC) -- INVALID > 3. (R, STATIC) -- statically reserved > 4. (R, DYNAMIC) -- dynamically reserved > > If we can't assume that `isReserved` and `isUnreserved` will be called on > valid `Resource` objects, we're forced to write them like this: > > ```cpp > bool isReserved(const Resource& resource) { return resource.role() != > "*"; } > bool isUnreserved(const Resource& resource) { return resource.role() == > "*" && resource.reservation_type() == STATIC; } > ``` > > It seems weird to me that one couldn't be implemented in terms of other. > > Alexander Rukletsov wrote: > I see you point with validations. Feel free to drop or update just that > specific `if` statement, whatever feels more correct for you. > > Regarding your reservation example. I would argue, that we have three > states here (reserved, unreserved, invalid), not two, and therefore > implementing `isReserved()` via `isUnreserved` is not trivial. > > ``` > bool isValid(const Resource& resource) { return !(resource.role() == "*" > && resource.reservation_type() == DYNAMIC); } > bool isReserved(const Resource& resource) { return resource.role() != > "*"; } > bool isUnreserved(const Resource& resource) { return (isValid() && > !isReserved(resource)); } > ```
Ok. I see where the discrepency is regarding the 3 reservation example. An `invalid` state however isn't a state we want to carry around within our system. It's a state incorrectly provided by the user that we want to reject right away via the `validate` function so that we don't have to worry about it. It would be great if our validation pipeline was enforced more strictly (for example, by performing validation during the construction of the C++ protobuf objects) rather than calling `validate` manually at the user input receiving site since it's easier to miss. But given what we have, the expectation is that we immediately validate any user input protobufs and proceed to work with the objects in their valid states. `Input` --`Validate`--> Our world where `isReserved` and `isUnreserved` are opposites of each other and `isValid` is always true. - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29742/#review72358 ----------------------------------------------------------- On Feb. 17, 2015, 8:08 p.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29742/ > ----------------------------------------------------------- > > (Updated Feb. 17, 2015, 8:08 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 their 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 ec7ec1356e745bb07484ae1755c9183b038043b3 > > Diff: https://reviews.apache.org/r/29742/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Michael Park > >
