> 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");
> > }
> > }
> > ```
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.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29742/#review72358
-----------------------------------------------------------
On Feb. 13, 2015, 10:59 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29742/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 10:59 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
>
>
