mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 52002: Added helper methods to determine types of disk resources.
Date Wed, 21 Sep 2016 22:54:07 GMT


> On Sept. 20, 2016, 3:29 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 276-278
> > <https://reviews.apache.org/r/52002/diff/2/?file=1504213#file1504213line276>
> >
> >     Would it be simpler to just have 
> >     
> >     ```
> >     static bool Resources::isMountDisk(const Resource& resource);
> >     static bool Resources::isPathDisk(const Resource& resource);
> >     ```
> >     
> >     ?
> >     
> >     We have already lost enumerability (or swtichability) given that the root disk
does not have a type. :)
> 
> Anindya Sinha wrote:
>     I prefer this way so as to not add functions when we add more disk types, say for
block devices.

Yes there's block devices but I don't imagine there being too many others. Otherwise I wouldn't
suggest this. :)

Beyond disks we really only have a few kinds of predefined resources so I think supporting
them directly wouldn't break the elegance of the API.

It would have been more consistent to if ROOT is a `DiskInfo::Source::Type` but otherwise
having `isRootDisk` and `getDiskSource()` feels inconsistent.


> On Sept. 20, 2016, 3:29 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 273-274
> > <https://reviews.apache.org/r/52002/diff/2/?file=1504213#file1504213line273>
> >
> >     Does it need to be a Try? i.e., if it's not a disk, it's not a root disk, right?
We have `validate()` to make sure resources are valid.
> 
> Anindya Sinha wrote:
>     If `resource.name() != "disk"`, we can skip this resource altogether. But if this
is a `disk` but not a root disk (ie. `resource.has_disk()` is `true`), then we also check
for `MOUNT` or `PATH`. So by returning a `Try<bool>`, we are able to decide if we should
continue with this resource or not. Ofcourse, we can check for `resource.has_disk()` outside
of these functions but adding that check makes this function self-contained.

Returning an Error on a simple predicate feels odd to me. You don't need to use an error to
capture that. If necessary `isDisk()` would have been better.

Similar examples can be found in the same class, e.g.,:

```
bool isPersistentVolume(const Resource& resource);
```

Plus we can use `filter()` on these predicates.

Having to first check if the resource is a disk and then decide whether to call getDiskSource()
on the caller side or having to deal the error returned by getDiskSource() sounds like to
reason we'll want to hide them.

The following is very simple right?

```
// By now we already know it's a disk, if it is a gpu we wouldn't call `isRootDisk()` even
if it returns an error, right?

if (Resources::isRootDisk()) {
...
} else if (Resources::isMountDisk()) {
...
} else {
  // Hypothetical TODO: Handle block devices.
  CHECK(Resources::isPathDisk());
  ...
}
```


> On Sept. 20, 2016, 3:29 p.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, line 920
> > <https://reviews.apache.org/r/52002/diff/2/?file=1504215#file1504215line920>
> >
> >     Not `!resource.has_disk()` but rather `!resource.disk().has_source()` right?
> >     
> >     See examples below.
> 
> Anindya Sinha wrote:
>     I think `!resource.has_disk()` is fine. Why should a root disk have `Persistence`
and/or `Volume`? `Persistence` is to define characteristics of a persistent volume, where
as `Volume` is how the disk resource is mounted in a container.

Oh I meant "Not **necessarily** `!resource.has_disk()`".

A root disk is defined as "disk without Diskinfo::Source", `!resource.has_disk() || !resource.disk().has_source()`
is a more direct translation of it.

Persistent add/or Volume are irrelevent but can be "on" the root disk, the disk is still a
root disk with them.


- Jiang Yan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/#review149739
-----------------------------------------------------------


On Sept. 20, 2016, 9:14 p.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52002/
> -----------------------------------------------------------
> 
> (Updated Sept. 20, 2016, 9:14 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
>     https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added helper methods to determine types of disk resources.
> 
> 
> Diffs
> -----
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/52002/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>


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