----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52002/#review150585 -----------------------------------------------------------
include/mesos/resources.hpp (lines 278 - 280) <https://reviews.apache.org/r/52002/#comment219998> 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. src/common/resources.cpp (line 848) <https://reviews.apache.org/r/52002/#comment219999> Don't do the CHECK here. Let's follow the `isPersistentVolume` example. However I agree we should check the resource name (which I overlooked). We can just embed it into the return: ``` return resource.name() == "disk" && (!resource.has_disk() || !resource.disk().has_source()); ``` Speaking of `isPersistentVolume()`, it makes sense to add `resource.name() == "disk" &&` to it as well. - Jiang Yan Xu On Oct. 3, 2016, 4:46 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52002/ > ----------------------------------------------------------- > > (Updated Oct. 3, 2016, 4:46 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 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 > >
