> On Oct. 5, 2016, 8:56 p.m., Jiang Yan Xu wrote: > > include/mesos/resources.hpp, lines 278-280 > > <https://reviews.apache.org/r/52002/diff/7/?file=1519241#file1519241line278> > > > > I still have reservations about this method. > > > > As we discussed, a predicate returning a `bool` is consistent with > > other similar methods and can be used more generally, e.g., > > > > ``` > > resources.filter([](const Resource& resource) { return > > isMountDisk(resource); }); > > ``` > > > > Your main concern (from our conversations) is that it's possible that > > we add more disk types and it would be unwieldy if we have 17 similar calls > > in the form of `bool Resources::isXYZ()`. > > > > My take is that, we currently have only a handful of predefined first > > class resource types (Mesos has special logic for them) and it's not > > unreasonble to have first class support (such as having an `isXYZ()` > > dedicated for it) for all of them. If in the future we have a lot of them, > > we should still provide first class support for **all** of them, but before > > this list gets too large we should probably improve the abstraction to > > support it in a different, more structured way, e.g., a `Disk` class for > > disks. > > > > --- > > > > The problems of adding this method right now include: > > > > ## 1. We shouldn't use Try::error when it's not really an error. > > > > Looking at > > > > ``` > > static Try<Resource::DiskInfo::Source::Type> getDiskSource(const > > Resource& resource); > > ``` > > > > I wouldn't know how to deal with the error, is it because the Resource > > is invalid? Is it not a disk? It being a valid disk but without a > > `Source::Type` is not one of the top things spring to mind. > > > > To get the disk type, it may be (slightly) better to have: > > > > ``` > > static Option<Resource::DiskInfo::Source::Type> getDiskSourceType(const > > Resource& resource); > > ``` > > > > With None meaning "there's no source type". But the problem is that > > with a generic `resource` argument, what if the resource is valid but not a > > disk? What if the resource is invalid? > > > > ## 2. The real problem is a flat Resource type with methods that make > > sense only to a special kind of Resource. > > > > Due to the different shapes of pre-defined resources, I find it > > unrealistic/difficult to use/implement methods that handle/expose all > > error/edge cases in one call. > > > > It would have made sense to have > > > > ``` > > Disk::Type Disk::getType(); > > ``` > > > > where we know disk is valid. `Disk` can be a subclass of C++ > > (non-protobuf) `Resource` which is guaranteed to be valid. > > > > --- > > > > For now, I think we should aim for consistency by sticking to the > > `isXYZ()` methods. It makes using and understanding of the methods easier > > both for the current users and for refactor in the future. (After all I > > think manging the complexity of current use and complexity of change is all > > that we are doing here). In most cases the client has to know if the > > Resource represents a (valid) disk first, they can do that and then use the > > set of `isXYZ()` to determine the type. > > > > --- > > > > > > Finally, we may not even need `isMountDisk()` or `isPathDisk` given how > > /r/51879/ is going. It may make sense to not specially treat PATH disk > > differently but let's disucss that in /r/51879/. With this review we can > > just have a `bool isRootDisk()` first if you like.
I removed the API `getDiskSource()` based on feedback from https://reviews.apache.org/r/51879/. - Anindya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52002/#review150585 ----------------------------------------------------------- On Oct. 6, 2016, 2 a.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52002/ > ----------------------------------------------------------- > > (Updated Oct. 6, 2016, 2 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 0774ff0669e831494d5b12b88e19dfa0a4a3f757 > src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a > src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 > > Diff: https://reviews.apache.org/r/52002/diff/ > > > Testing > ------- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
