Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/
---

(Updated May 24, 2017, 7:56 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 (updated)
-

  include/mesos/resources.hpp 74583e3c784f22b45cce81f07c5a69b339e1684d 
  include/mesos/v1/resources.hpp 132ef3eb03d335774ba13ecb39045128d0476954 
  src/common/resources.cpp 1d07f1a049ba3bb3f5d78f05031f33ba97e07e8c 
  src/tests/resources_tests.cpp 5dcbce2dd4944194b59790551929d6d75b805ba3 
  src/v1/resources.cpp 1bb5d0741c9f9ea59f34e92159a335661019444d 


Diff: https://reviews.apache.org/r/52002/diff/13/

Changes: https://reviews.apache.org/r/52002/diff/12-13/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Joris Van Remoortere]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/#review167527
---



Closing this review due to inactivity. Please see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On Dec. 18, 2016, 8:38 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52002/
> ---
> 
> (Updated Dec. 18, 2016, 8:38 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 c2e6d6680d9687f0bbfb73e84ffb1457072c4412 
>   include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1 
>   src/common/resources.cpp 7dbeefc8fc159f28dc1542ad6a761213c65ff226 
>   src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
>   src/v1/resources.cpp 859f7388035b855bf0cb60d5cbe746e6871bf833 
> 
> 
> Diff: https://reviews.apache.org/r/52002/diff/12/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/
---

(Updated Dec. 18, 2016, 8:38 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOS-6062
https://issues.apache.org/jira/browse/MESOS-6062


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

  include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412 
  include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1 
  src/common/resources.cpp 7dbeefc8fc159f28dc1542ad6a761213c65ff226 
  src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
  src/v1/resources.cpp 859f7388035b855bf0cb60d5cbe746e6871bf833 

Diff: https://reviews.apache.org/r/52002/diff/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/
---

(Updated Dec. 10, 2016, 1:19 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebase.


Bugs: MESOS-6062
https://issues.apache.org/jira/browse/MESOS-6062


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

  include/mesos/resources.hpp f569c931ff7db8d51dfd7c96f4f2addab05df85d 
  include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7 
  src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f 
  src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 
  src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9 

Diff: https://reviews.apache.org/r/52002/diff/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/
---

(Updated Nov. 21, 2016, 7:18 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

addressed review comments.


Bugs: MESOS-6062
https://issues.apache.org/jira/browse/MESOS-6062


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

  include/mesos/resources.hpp f569c931ff7db8d51dfd7c96f4f2addab05df85d 
  include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7 
  src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f 
  src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 
  src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9 

Diff: https://reviews.apache.org/r/52002/diff/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/#review156362
---


Fix it, then Ship it!





src/common/resources.cpp (lines 792 - 793)


This indentation looks odd.

Let's do:

```
  return resource.name() == "disk" &&
 resource.has_disk() && 
 resource.disk().has_persistence();
```



src/common/resources.cpp (lines 835 - 836)


Style suggestion:

```
  return resource.name() == "disk" &&
 (!resource.has_disk() || !resource.disk().has_source());
```


- Jiang Yan Xu


On Oct. 25, 2016, 11:19 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52002/
> ---
> 
> (Updated Oct. 25, 2016, 11:19 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 f569c931ff7db8d51dfd7c96f4f2addab05df85d 
>   include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7 
>   src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f 
>   src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 
>   src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9 
> 
> Diff: https://reviews.apache.org/r/52002/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

> On Oct. 5, 2016, 8:56 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 278-280
> > 
> >
> > 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 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 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: 

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/
---

(Updated Oct. 25, 2016, 6:19 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments.


Bugs: MESOS-6062
https://issues.apache.org/jira/browse/MESOS-6062


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

  include/mesos/resources.hpp f569c931ff7db8d51dfd7c96f4f2addab05df85d 
  include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7 
  src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f 
  src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 
  src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9 

Diff: https://reviews.apache.org/r/52002/diff/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

> On Oct. 2, 2016, 3:14 a.m., Guangya Liu wrote:
> > src/tests/resources_tests.cpp, line 2182
> > 
> >
> > Here we should keep 4 space start from the `(` above or else you can:
> > 
> > ```
> > ASSERT_TRUE(
> > Resources::isRootDisk(
> > createDiskResource("100", "role1", None(), None(;
> > ```
> > 
> > Ditto for others which has two `(` in one line.
> 
> Anindya Sinha wrote:
> Can you point to the specific style from the style guide 
> (http://mesos.apache.org/documentation/latest/c++-style-guide/) which is 
> being violated here?
> I agree your option would satisfy the style guide but am curious why do 
> you think the current version violates it. AFAICT, there is no specific rule 
> for 2 "(" in the same line.
> 
> Guangya Liu wrote:
> I did not found this in C++ style, but in a review here 
> https://reviews.apache.org/r/52020/

I think I would leave it the way it is since there are numerous places where 
this pattern is used. As one example: 
https://github.com/apache/mesos/blob/master/src/tests/persistent_volume_tests.cpp#L539-L541


- Anindya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/#review151126
---


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

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

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


Changes
---

Rebased.


Bugs: MESOS-6062
https://issues.apache.org/jira/browse/MESOS-6062


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

  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

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/#review150585
---




include/mesos/resources.hpp (lines 278 - 280)


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


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: 

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Guangya Liu]

> On 十月 2, 2016, 3:14 a.m., Guangya Liu wrote:
> > src/tests/resources_tests.cpp, line 2182
> > 
> >
> > Here we should keep 4 space start from the `(` above or else you can:
> > 
> > ```
> > ASSERT_TRUE(
> > Resources::isRootDisk(
> > createDiskResource("100", "role1", None(), None(;
> > ```
> > 
> > Ditto for others which has two `(` in one line.
> 
> Anindya Sinha wrote:
> Can you point to the specific style from the style guide 
> (http://mesos.apache.org/documentation/latest/c++-style-guide/) which is 
> being violated here?
> I agree your option would satisfy the style guide but am curious why do 
> you think the current version violates it. AFAICT, there is no specific rule 
> for 2 "(" in the same line.

I did not found this in C++ style, but in a review here 
https://reviews.apache.org/r/52020/


- Guangya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/#review151126
---


On 十月 3, 2016, 11:46 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52002/
> ---
> 
> (Updated 十月 3, 2016, 11: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
> 
>

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/
---

(Updated Oct. 3, 2016, 11:46 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments, and rebased.


Bugs: MESOS-6062
https://issues.apache.org/jira/browse/MESOS-6062


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

  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

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

> On Oct. 2, 2016, 3:14 a.m., Guangya Liu wrote:
> > src/tests/resources_tests.cpp, line 2182
> > 
> >
> > Here we should keep 4 space start from the `(` above or else you can:
> > 
> > ```
> > ASSERT_TRUE(
> > Resources::isRootDisk(
> > createDiskResource("100", "role1", None(), None(;
> > ```
> > 
> > Ditto for others which has two `(` in one line.

Can you point to the specific style from the style guide 
(http://mesos.apache.org/documentation/latest/c++-style-guide/) which is being 
violated here?
I agree your option would satisfy the style guide but am curious why do you 
think the current version violates it. AFAICT, there is no specific rule for 2 
"(" in the same line.


- Anindya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/#review151126
---


On Sept. 28, 2016, 7:24 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52002/
> ---
> 
> (Updated Sept. 28, 2016, 7:24 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
> 
>

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/#review151126
---




src/common/resources.cpp (lines 857 - 860)


I think we should kill this as we already have the validation in 
`isRootDisk` and the resource will always have disk source at here.



src/tests/resources_tests.cpp (line 2182)


Here we should keep 4 space start from the `(` above or else you can:

```
ASSERT_TRUE(
Resources::isRootDisk(
createDiskResource("100", "role1", None(), None(;
```

Ditto for others which has two `(` in one line.


- Guangya Liu


On 九月 28, 2016, 7:24 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52002/
> ---
> 
> (Updated 九月 28, 2016, 7:24 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
> 
>

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/
---

(Updated Sept. 28, 2016, 7:24 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOS-6062
https://issues.apache.org/jira/browse/MESOS-6062


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

  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

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/
---

(Updated Sept. 26, 2016, 8:58 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOS-6062
https://issues.apache.org/jira/browse/MESOS-6062


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

  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 (updated)
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/
---

(Updated Sept. 26, 2016, 6:48 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Modified `isRootDisk()` to return `bool` instead of `Try`.


Bugs: MESOS-6062
https://issues.apache.org/jira/browse/MESOS-6062


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

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


Thanks,

Anindya Sinha

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

> On Sept. 20, 2016, 10:29 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 276-278
> > 
> >
> > 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
> > 
> >
> > 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`, 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
> > 

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Jiang Yan Xu]

> On Sept. 20, 2016, 3:29 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 276-278
> > 
> >
> > 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
> > 
> >
> > 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`, 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
> > 
> >
> > 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,
> 

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

> On Sept. 20, 2016, 10:29 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 273-274
> > 
> >
> > 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.

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`, 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.


> On Sept. 20, 2016, 10:29 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 276-278
> > 
> >
> > 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. :)

I prefer this way so as to not add functions when we add more disk types, say 
for block devices.


> On Sept. 20, 2016, 10:29 p.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, line 920
> > 
> >
> > Not `!resource.has_disk()` but rather `!resource.disk().has_source()` 
> > right?
> > 
> > See examples below.

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.


> On Sept. 20, 2016, 10:29 p.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, lines 924-943
> > 
> >
> > It seems to be cleaner to implement the following instead:
> > 
> > ```
> > static bool Resources::isMountDisk(const Resource& resource)
> > {
> >   return resource.has_disk() && 
> >  resource.disk().has_source() &&
> >  resource.disk().source().type() == 
> > Resource::DiskInfo::Source::MOUNT;
> > }
> > 
> > static bool Resources::isPathDisk(const Resource& resource)
> > {
> >   return resource.has_disk() && 
> >  resource.disk().has_source() &&
> >  resource.disk().source().type() == 
> > Resource::DiskInfo::Source::PATH;
> > }
> > ```

See my response above. I think having a single function to encapsulate all 
source types is fine.


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

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

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


Changes
---

Rebased.


Bugs: MESOS-6062
https://issues.apache.org/jira/browse/MESOS-6062


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

  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

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/#review149739
---




include/mesos/resources.hpp (lines 273 - 274)


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.



include/mesos/resources.hpp (lines 276 - 278)


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. :)



src/common/resources.cpp (line 920)


Not `!resource.has_disk()` but rather `!resource.disk().has_source()` right?

See examples below.



src/common/resources.cpp (lines 924 - 943)


It seems to be cleaner to implement the following instead:

```
static bool Resources::isMountDisk(const Resource& resource)
{
  return resource.has_disk() && 
 resource.disk().has_source() &&
 resource.disk().source().type() == 
Resource::DiskInfo::Source::MOUNT;
}

static bool Resources::isPathDisk(const Resource& resource)
{
  return resource.has_disk() && 
 resource.disk().has_source() &&
 resource.disk().source().type() == 
Resource::DiskInfo::Source::PATH;
}
```


- Jiang Yan Xu


On Sept. 19, 2016, 3:43 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52002/
> ---
> 
> (Updated Sept. 19, 2016, 3:43 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
> 
>

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/
---

(Updated Sept. 19, 2016, 10:43 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Minor updates based on review comments.


Bugs: MESOS-6062
https://issues.apache.org/jira/browse/MESOS-6062


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

  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

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/#review149440
---




src/common/resources.cpp (lines 930 - 931)


```
return Error("Resource " + stringify(resource) +
 " is not a PATH or MOUNT disk");
```



src/tests/resources_tests.cpp (lines 2181 - 2196)


There is no need to define temp variables here, you can refer to 
https://github.com/apache/mesos/blob/master/src/tests/resources_tests.cpp#L1922-L1928

Ditto for the following



src/v1/resources.cpp (lines 932 - 933)


```
return Error("Resource " + stringify(resource) +
 " is not a PATH or MOUNT disk");
```


- Guangya Liu


On 九月 19, 2016, 5 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52002/
> ---
> 
> (Updated 九月 19, 2016, 5 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
> 
>

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/
---

(Updated Sept. 19, 2016, 5 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Add reviewer.


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

Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52002/
---

Review request for mesos.


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