mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Park" <mcyp...@gmail.com>
Subject Re: Review Request 29742: Added utility functions to determine types of resources.
Date Fri, 13 Feb 2015 23:18:46 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message