> On Sept. 20, 2016, 10: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. > > Jiang Yan Xu wrote: > 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.
Exactly. If you can achieve all current and future disk source types by a single API, I am not sure why you would need multiple functions? Just as an analogy, for `Resource::DiskInfo::Source`, protobuf exposes a function `type()` that returns one of `Resource::DiskInfo::Source::MOUNT` or `Resource::DiskInfo::Source::PATH`; and not functions like `isMount()` and `isPath()`? > On Sept. 20, 2016, 10: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. > > Jiang Yan Xu wrote: > 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 `bool isPersistentVolume(const Resource& resource);`: I understand that the call sites from where this is called already ensures it is a disk resource, but if we can embed in the API, I think that is better. > On "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.": I am not sure I understand this. The caller calls `isRootDisk()` or `getDiskSource()`. If it gets an Error, it is an invalid resource within the context of the called APIs, otherwise it processes that resource. The caller does not need to check if the resource is a disk. Going on the same principles, I believe verifying the resource is a disk before calling `isRootDisk()`, etc. is a better option. It keeps the API self contained. Although all the call sites would ensure that this is a disk resource, it does not need to if the API itself encompasses that. eg. If the input resource is actually a "cpus" resource, then `isRootDisk()` would return `true` if we do not check for the actual resource type. Regarding returning an Error when the resource name is not disk: I believe it can be seen as an Error since the caller wanted to determine if the resource is a root disk for a non-disk resource... again, keeps everything self contained within the same function. > On Sept. 20, 2016, 10: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. > > Jiang Yan Xu wrote: > 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. Ok. I see that now in the documentation. I will update accordingly. However, I am not sure how a root disk can have a `Resource::DiskInfo` (ie. it has one of `Persistence` or `Volume`). - Anindya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52002/#review149739 ----------------------------------------------------------- On Sept. 21, 2016, 4:14 a.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52002/ > ----------------------------------------------------------- > > (Updated Sept. 21, 2016, 4:14 a.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 > >
