mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anindya Sinha <anindya_si...@apple.com>
Subject Re: Review Request 52002: Added helper methods to determine types of disk resources.
Date Tue, 25 Oct 2016 18:18:52 GMT


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


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