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 Wed, 11 Feb 2015 23:23:49 GMT

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


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