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