Re: Review Request 50836: Made add/subtract resource object as private method.

[Guangya Liu]

> On 八月 16, 2016, 11:44 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 59
> > 
> >
> > I think it would still work if you keep the `static` keyword right?
> > 
> > The `friend` keyword wouldn't allow you to specify `static` but here 
> > you can. 
> > 
> > Then in the cpp files you don't have to remove the `static` keyword on 
> > the definition.
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, but it still does not work as your proposal by adding 
> `static` for the `convertJSON` definition in cpp file and forward 
> declarations in hpp file as the `friend Try 
> internal::convertJSON()` will still think this method is not a `static` 
> method and this will cause build error as following.
> 
> ```
> In file included from ../../src/common/resources_utils.cpp:21:
> In file included from ../../src/common/resources_utils.hpp:21:
> ../../include/mesos/resources.hpp:59:25: error: unused function 
> 'convertJSON' [-Werror,-Wunused-function]
>   static Try convertJSON(
> ^
> 1 error generated.
> make[2]: *** [common/libmesos_no_3rdparty_la-resources_utils.lo] Error 1
> ```
> 
> Another is that I have posted some comments in 
> https://reviews.apache.org/r/50568/ and proposing keep `add` as public, can 
> you please take a look and show your comments if any.
> 
> The following are comments cited from https://reviews.apache.org/r/50568/
> 
> ```
> @Ben and @Yan, I found another place where we can enhance for allocator 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274
>  and this was only used by quota now, I think we can use add here. The 
> problem is that @Yan porposed to make the add as a private method and I've 
> posted a patch here https://reviews.apache.org/r/50836/diff/1#index_header , 
> but after a second thought, perhaps we can enable framework developers or 
> other users call this add API directly. As sometimes, the user may want to 
> traverse all Resources object and make some update for each Resource object 
> and then add those Resource again just like what we did in allocator for now. 
> So what about keep add as a public method?
> ```
> 
> Jiang Yan Xu wrote:
> My apologies. Looks like it's problematic to declare methods as static in 
> the header anyways. To keep the method internal to resources.cpp you can 
> define it as `inline` and keep the forward and friend declarations without 
> `inline` or `static`.
> 
> On the subject of keeping `add()/subtract()` public so some external code 
> can call them, I feel that this is a limitation of the current 
> Resource-related abstractions. If we keep the method public, we have to count 
> on the callers to know a lot about the internal implementation of `Resources` 
> and use it carefully. This is in general against information hiding and loose 
> coupling. 
> 
> Ideally if we have a C++ `Resource` abstraction that handles validation 
> and we know that a (non-proto) `Resource` object must be valid, we can make 
> `+=` validation free but until then, I understand that there are places in 
> Mesos codebase already with compromises like this. I think we need to weigh 
> the performance gains we can get against the drawback in the cleanliness of 
> the abstraction. We can put a hold on this review and keep the methods public 
> for now if you are thinking about using them to improve performance and 
> conducting benchmarks to compare. :)
> 
> What are your thoughts @BenM?
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, yes, the `inline` works for this.
> 
> For keeping `add()/subtract()`, agree that we need a benchmark test for 
> this. The current 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274
>  was only used by quota and there is no benchmark test related for quota now, 
> perhaps we need to add one such case for benchmark test, @Ben and @Jiang Yan, 
> what do you think?
> 
> Another point is from 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1283-L1295
>  , if we have thousandes of agents and some quotas, I think the performance 
> will improve much based on the test result of 
> https://reviews.apache.org/r/50557/
> 
> Guangya Liu wrote:
> Had some offline discussion with Ben, we will make those two methods 
> add/subtract as private for now as it only impacts quota related logic in 
> allocator. And we can add some other wrapper functions to expose those two 
> APIs if they are needed in future. Also the benchmark test for `Quota` is not 
> priority, so there is no need to add them for now.
> 
> Jiang Yan Xu wrote:
> Alright I'll commit this then. So you haven't found other places on the 
> critical path that loops through a Resources object and add 

Re: Review Request 50836: Made add/subtract resource object as private method.

[Guangya Liu]

> On 八月 16, 2016, 11:44 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 59
> > 
> >
> > I think it would still work if you keep the `static` keyword right?
> > 
> > The `friend` keyword wouldn't allow you to specify `static` but here 
> > you can. 
> > 
> > Then in the cpp files you don't have to remove the `static` keyword on 
> > the definition.
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, but it still does not work as your proposal by adding 
> `static` for the `convertJSON` definition in cpp file and forward 
> declarations in hpp file as the `friend Try 
> internal::convertJSON()` will still think this method is not a `static` 
> method and this will cause build error as following.
> 
> ```
> In file included from ../../src/common/resources_utils.cpp:21:
> In file included from ../../src/common/resources_utils.hpp:21:
> ../../include/mesos/resources.hpp:59:25: error: unused function 
> 'convertJSON' [-Werror,-Wunused-function]
>   static Try convertJSON(
> ^
> 1 error generated.
> make[2]: *** [common/libmesos_no_3rdparty_la-resources_utils.lo] Error 1
> ```
> 
> Another is that I have posted some comments in 
> https://reviews.apache.org/r/50568/ and proposing keep `add` as public, can 
> you please take a look and show your comments if any.
> 
> The following are comments cited from https://reviews.apache.org/r/50568/
> 
> ```
> @Ben and @Yan, I found another place where we can enhance for allocator 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274
>  and this was only used by quota now, I think we can use add here. The 
> problem is that @Yan porposed to make the add as a private method and I've 
> posted a patch here https://reviews.apache.org/r/50836/diff/1#index_header , 
> but after a second thought, perhaps we can enable framework developers or 
> other users call this add API directly. As sometimes, the user may want to 
> traverse all Resources object and make some update for each Resource object 
> and then add those Resource again just like what we did in allocator for now. 
> So what about keep add as a public method?
> ```
> 
> Jiang Yan Xu wrote:
> My apologies. Looks like it's problematic to declare methods as static in 
> the header anyways. To keep the method internal to resources.cpp you can 
> define it as `inline` and keep the forward and friend declarations without 
> `inline` or `static`.
> 
> On the subject of keeping `add()/subtract()` public so some external code 
> can call them, I feel that this is a limitation of the current 
> Resource-related abstractions. If we keep the method public, we have to count 
> on the callers to know a lot about the internal implementation of `Resources` 
> and use it carefully. This is in general against information hiding and loose 
> coupling. 
> 
> Ideally if we have a C++ `Resource` abstraction that handles validation 
> and we know that a (non-proto) `Resource` object must be valid, we can make 
> `+=` validation free but until then, I understand that there are places in 
> Mesos codebase already with compromises like this. I think we need to weigh 
> the performance gains we can get against the drawback in the cleanliness of 
> the abstraction. We can put a hold on this review and keep the methods public 
> for now if you are thinking about using them to improve performance and 
> conducting benchmarks to compare. :)
> 
> What are your thoughts @BenM?
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, yes, the `inline` works for this.
> 
> For keeping `add()/subtract()`, agree that we need a benchmark test for 
> this. The current 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274
>  was only used by quota and there is no benchmark test related for quota now, 
> perhaps we need to add one such case for benchmark test, @Ben and @Jiang Yan, 
> what do you think?
> 
> Another point is from 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1283-L1295
>  , if we have thousandes of agents and some quotas, I think the performance 
> will improve much based on the test result of 
> https://reviews.apache.org/r/50557/
> 
> Guangya Liu wrote:
> Had some offline discussion with Ben, we will make those two methods 
> add/subtract as private for now as it only impacts quota related logic in 
> allocator. And we can add some other wrapper functions to expose those two 
> APIs if they are needed in future. Also the benchmark test for `Quota` is not 
> priority, so there is no need to add them for now.
> 
> Jiang Yan Xu wrote:
> Alright I'll commit this then. So you haven't found other places on the 
> critical path that loops through a Resources object and add 

Re: Review Request 50836: Made add/subtract resource object as private method.

[Jiang Yan Xu]

> On Aug. 16, 2016, 4:44 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 59
> > 
> >
> > I think it would still work if you keep the `static` keyword right?
> > 
> > The `friend` keyword wouldn't allow you to specify `static` but here 
> > you can. 
> > 
> > Then in the cpp files you don't have to remove the `static` keyword on 
> > the definition.
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, but it still does not work as your proposal by adding 
> `static` for the `convertJSON` definition in cpp file and forward 
> declarations in hpp file as the `friend Try 
> internal::convertJSON()` will still think this method is not a `static` 
> method and this will cause build error as following.
> 
> ```
> In file included from ../../src/common/resources_utils.cpp:21:
> In file included from ../../src/common/resources_utils.hpp:21:
> ../../include/mesos/resources.hpp:59:25: error: unused function 
> 'convertJSON' [-Werror,-Wunused-function]
>   static Try convertJSON(
> ^
> 1 error generated.
> make[2]: *** [common/libmesos_no_3rdparty_la-resources_utils.lo] Error 1
> ```
> 
> Another is that I have posted some comments in 
> https://reviews.apache.org/r/50568/ and proposing keep `add` as public, can 
> you please take a look and show your comments if any.
> 
> The following are comments cited from https://reviews.apache.org/r/50568/
> 
> ```
> @Ben and @Yan, I found another place where we can enhance for allocator 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274
>  and this was only used by quota now, I think we can use add here. The 
> problem is that @Yan porposed to make the add as a private method and I've 
> posted a patch here https://reviews.apache.org/r/50836/diff/1#index_header , 
> but after a second thought, perhaps we can enable framework developers or 
> other users call this add API directly. As sometimes, the user may want to 
> traverse all Resources object and make some update for each Resource object 
> and then add those Resource again just like what we did in allocator for now. 
> So what about keep add as a public method?
> ```
> 
> Jiang Yan Xu wrote:
> My apologies. Looks like it's problematic to declare methods as static in 
> the header anyways. To keep the method internal to resources.cpp you can 
> define it as `inline` and keep the forward and friend declarations without 
> `inline` or `static`.
> 
> On the subject of keeping `add()/subtract()` public so some external code 
> can call them, I feel that this is a limitation of the current 
> Resource-related abstractions. If we keep the method public, we have to count 
> on the callers to know a lot about the internal implementation of `Resources` 
> and use it carefully. This is in general against information hiding and loose 
> coupling. 
> 
> Ideally if we have a C++ `Resource` abstraction that handles validation 
> and we know that a (non-proto) `Resource` object must be valid, we can make 
> `+=` validation free but until then, I understand that there are places in 
> Mesos codebase already with compromises like this. I think we need to weigh 
> the performance gains we can get against the drawback in the cleanliness of 
> the abstraction. We can put a hold on this review and keep the methods public 
> for now if you are thinking about using them to improve performance and 
> conducting benchmarks to compare. :)
> 
> What are your thoughts @BenM?
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, yes, the `inline` works for this.
> 
> For keeping `add()/subtract()`, agree that we need a benchmark test for 
> this. The current 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274
>  was only used by quota and there is no benchmark test related for quota now, 
> perhaps we need to add one such case for benchmark test, @Ben and @Jiang Yan, 
> what do you think?
> 
> Another point is from 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1283-L1295
>  , if we have thousandes of agents and some quotas, I think the performance 
> will improve much based on the test result of 
> https://reviews.apache.org/r/50557/
> 
> Guangya Liu wrote:
> Had some offline discussion with Ben, we will make those two methods 
> add/subtract as private for now as it only impacts quota related logic in 
> allocator. And we can add some other wrapper functions to expose those two 
> APIs if they are needed in future. Also the benchmark test for `Quota` is not 
> priority, so there is no need to add them for now.
> 
> Jiang Yan Xu wrote:
> Alright I'll commit this then. So you haven't found other places on the 
> critical path that loops through a Resources object and add 

Re: Review Request 50836: Made add/subtract resource object as private method.

[Guangya Liu]

> On 八月 16, 2016, 11:44 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 59
> > 
> >
> > I think it would still work if you keep the `static` keyword right?
> > 
> > The `friend` keyword wouldn't allow you to specify `static` but here 
> > you can. 
> > 
> > Then in the cpp files you don't have to remove the `static` keyword on 
> > the definition.
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, but it still does not work as your proposal by adding 
> `static` for the `convertJSON` definition in cpp file and forward 
> declarations in hpp file as the `friend Try 
> internal::convertJSON()` will still think this method is not a `static` 
> method and this will cause build error as following.
> 
> ```
> In file included from ../../src/common/resources_utils.cpp:21:
> In file included from ../../src/common/resources_utils.hpp:21:
> ../../include/mesos/resources.hpp:59:25: error: unused function 
> 'convertJSON' [-Werror,-Wunused-function]
>   static Try convertJSON(
> ^
> 1 error generated.
> make[2]: *** [common/libmesos_no_3rdparty_la-resources_utils.lo] Error 1
> ```
> 
> Another is that I have posted some comments in 
> https://reviews.apache.org/r/50568/ and proposing keep `add` as public, can 
> you please take a look and show your comments if any.
> 
> The following are comments cited from https://reviews.apache.org/r/50568/
> 
> ```
> @Ben and @Yan, I found another place where we can enhance for allocator 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274
>  and this was only used by quota now, I think we can use add here. The 
> problem is that @Yan porposed to make the add as a private method and I've 
> posted a patch here https://reviews.apache.org/r/50836/diff/1#index_header , 
> but after a second thought, perhaps we can enable framework developers or 
> other users call this add API directly. As sometimes, the user may want to 
> traverse all Resources object and make some update for each Resource object 
> and then add those Resource again just like what we did in allocator for now. 
> So what about keep add as a public method?
> ```
> 
> Jiang Yan Xu wrote:
> My apologies. Looks like it's problematic to declare methods as static in 
> the header anyways. To keep the method internal to resources.cpp you can 
> define it as `inline` and keep the forward and friend declarations without 
> `inline` or `static`.
> 
> On the subject of keeping `add()/subtract()` public so some external code 
> can call them, I feel that this is a limitation of the current 
> Resource-related abstractions. If we keep the method public, we have to count 
> on the callers to know a lot about the internal implementation of `Resources` 
> and use it carefully. This is in general against information hiding and loose 
> coupling. 
> 
> Ideally if we have a C++ `Resource` abstraction that handles validation 
> and we know that a (non-proto) `Resource` object must be valid, we can make 
> `+=` validation free but until then, I understand that there are places in 
> Mesos codebase already with compromises like this. I think we need to weigh 
> the performance gains we can get against the drawback in the cleanliness of 
> the abstraction. We can put a hold on this review and keep the methods public 
> for now if you are thinking about using them to improve performance and 
> conducting benchmarks to compare. :)
> 
> What are your thoughts @BenM?
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, yes, the `inline` works for this.
> 
> For keeping `add()/subtract()`, agree that we need a benchmark test for 
> this. The current 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274
>  was only used by quota and there is no benchmark test related for quota now, 
> perhaps we need to add one such case for benchmark test, @Ben and @Jiang Yan, 
> what do you think?
> 
> Another point is from 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1283-L1295
>  , if we have thousandes of agents and some quotas, I think the performance 
> will improve much based on the test result of 
> https://reviews.apache.org/r/50557/
> 
> Guangya Liu wrote:
> Had some offline discussion with Ben, we will make those two methods 
> add/subtract as private for now as it only impacts quota related logic in 
> allocator. And we can add some other wrapper functions to expose those two 
> APIs if they are needed in future. Also the benchmark test for `Quota` is not 
> priority, so there is no need to add them for now.
> 
> Jiang Yan Xu wrote:
> Alright I'll commit this then. So you haven't found other places on the 
> critical path that loops through a Resources object and add 

Re: Review Request 50836: Made add/subtract resource object as private method.

[Jiang Yan Xu]

> On Aug. 16, 2016, 4:44 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 59
> > 
> >
> > I think it would still work if you keep the `static` keyword right?
> > 
> > The `friend` keyword wouldn't allow you to specify `static` but here 
> > you can. 
> > 
> > Then in the cpp files you don't have to remove the `static` keyword on 
> > the definition.
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, but it still does not work as your proposal by adding 
> `static` for the `convertJSON` definition in cpp file and forward 
> declarations in hpp file as the `friend Try 
> internal::convertJSON()` will still think this method is not a `static` 
> method and this will cause build error as following.
> 
> ```
> In file included from ../../src/common/resources_utils.cpp:21:
> In file included from ../../src/common/resources_utils.hpp:21:
> ../../include/mesos/resources.hpp:59:25: error: unused function 
> 'convertJSON' [-Werror,-Wunused-function]
>   static Try convertJSON(
> ^
> 1 error generated.
> make[2]: *** [common/libmesos_no_3rdparty_la-resources_utils.lo] Error 1
> ```
> 
> Another is that I have posted some comments in 
> https://reviews.apache.org/r/50568/ and proposing keep `add` as public, can 
> you please take a look and show your comments if any.
> 
> The following are comments cited from https://reviews.apache.org/r/50568/
> 
> ```
> @Ben and @Yan, I found another place where we can enhance for allocator 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274
>  and this was only used by quota now, I think we can use add here. The 
> problem is that @Yan porposed to make the add as a private method and I've 
> posted a patch here https://reviews.apache.org/r/50836/diff/1#index_header , 
> but after a second thought, perhaps we can enable framework developers or 
> other users call this add API directly. As sometimes, the user may want to 
> traverse all Resources object and make some update for each Resource object 
> and then add those Resource again just like what we did in allocator for now. 
> So what about keep add as a public method?
> ```
> 
> Jiang Yan Xu wrote:
> My apologies. Looks like it's problematic to declare methods as static in 
> the header anyways. To keep the method internal to resources.cpp you can 
> define it as `inline` and keep the forward and friend declarations without 
> `inline` or `static`.
> 
> On the subject of keeping `add()/subtract()` public so some external code 
> can call them, I feel that this is a limitation of the current 
> Resource-related abstractions. If we keep the method public, we have to count 
> on the callers to know a lot about the internal implementation of `Resources` 
> and use it carefully. This is in general against information hiding and loose 
> coupling. 
> 
> Ideally if we have a C++ `Resource` abstraction that handles validation 
> and we know that a (non-proto) `Resource` object must be valid, we can make 
> `+=` validation free but until then, I understand that there are places in 
> Mesos codebase already with compromises like this. I think we need to weigh 
> the performance gains we can get against the drawback in the cleanliness of 
> the abstraction. We can put a hold on this review and keep the methods public 
> for now if you are thinking about using them to improve performance and 
> conducting benchmarks to compare. :)
> 
> What are your thoughts @BenM?
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, yes, the `inline` works for this.
> 
> For keeping `add()/subtract()`, agree that we need a benchmark test for 
> this. The current 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274
>  was only used by quota and there is no benchmark test related for quota now, 
> perhaps we need to add one such case for benchmark test, @Ben and @Jiang Yan, 
> what do you think?
> 
> Another point is from 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1283-L1295
>  , if we have thousandes of agents and some quotas, I think the performance 
> will improve much based on the test result of 
> https://reviews.apache.org/r/50557/
> 
> Guangya Liu wrote:
> Had some offline discussion with Ben, we will make those two methods 
> add/subtract as private for now as it only impacts quota related logic in 
> allocator. And we can add some other wrapper functions to expose those two 
> APIs if they are needed in future. Also the benchmark test for `Quota` is not 
> priority, so there is no need to add them for now.
> 
> Jiang Yan Xu wrote:
> Alright I'll commit this then. So you haven't found other places on the 
> critical path that loops through a Resources object and add 

Re: Review Request 50836: Made add/subtract resource object as private method.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50836]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Aug. 23, 2016, 10:19 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50836/
> ---
> 
> (Updated Aug. 23, 2016, 10:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not want people call add/subtract resoure object directly,
> so should make those methods as private.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp bdbe8ea03a50cd1361640bde13a2342909723fc5 
>   include/mesos/v1/resources.hpp c05cb634c7a5add78da00cb84fc75d3472a341bc 
>   src/common/resources.cpp 96b4c39507bdb9b88aca5c2178b88057a5fc1881 
>   src/v1/resources.cpp 3cc7580d5567370530c53759713be05c369bf298 
> 
> Diff: https://reviews.apache.org/r/50836/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh --gtest_filter="ResourcesTest.*"
> ./bin/mesos-tests.sh --gtest_filter="SharedResourcesTest.*"
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 50836: Made add/subtract resource object as private method.

[Guangya Liu]

> On 八月 16, 2016, 11:44 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 59
> > 
> >
> > I think it would still work if you keep the `static` keyword right?
> > 
> > The `friend` keyword wouldn't allow you to specify `static` but here 
> > you can. 
> > 
> > Then in the cpp files you don't have to remove the `static` keyword on 
> > the definition.
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, but it still does not work as your proposal by adding 
> `static` for the `convertJSON` definition in cpp file and forward 
> declarations in hpp file as the `friend Try 
> internal::convertJSON()` will still think this method is not a `static` 
> method and this will cause build error as following.
> 
> ```
> In file included from ../../src/common/resources_utils.cpp:21:
> In file included from ../../src/common/resources_utils.hpp:21:
> ../../include/mesos/resources.hpp:59:25: error: unused function 
> 'convertJSON' [-Werror,-Wunused-function]
>   static Try convertJSON(
> ^
> 1 error generated.
> make[2]: *** [common/libmesos_no_3rdparty_la-resources_utils.lo] Error 1
> ```
> 
> Another is that I have posted some comments in 
> https://reviews.apache.org/r/50568/ and proposing keep `add` as public, can 
> you please take a look and show your comments if any.
> 
> The following are comments cited from https://reviews.apache.org/r/50568/
> 
> ```
> @Ben and @Yan, I found another place where we can enhance for allocator 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274
>  and this was only used by quota now, I think we can use add here. The 
> problem is that @Yan porposed to make the add as a private method and I've 
> posted a patch here https://reviews.apache.org/r/50836/diff/1#index_header , 
> but after a second thought, perhaps we can enable framework developers or 
> other users call this add API directly. As sometimes, the user may want to 
> traverse all Resources object and make some update for each Resource object 
> and then add those Resource again just like what we did in allocator for now. 
> So what about keep add as a public method?
> ```
> 
> Jiang Yan Xu wrote:
> My apologies. Looks like it's problematic to declare methods as static in 
> the header anyways. To keep the method internal to resources.cpp you can 
> define it as `inline` and keep the forward and friend declarations without 
> `inline` or `static`.
> 
> On the subject of keeping `add()/subtract()` public so some external code 
> can call them, I feel that this is a limitation of the current 
> Resource-related abstractions. If we keep the method public, we have to count 
> on the callers to know a lot about the internal implementation of `Resources` 
> and use it carefully. This is in general against information hiding and loose 
> coupling. 
> 
> Ideally if we have a C++ `Resource` abstraction that handles validation 
> and we know that a (non-proto) `Resource` object must be valid, we can make 
> `+=` validation free but until then, I understand that there are places in 
> Mesos codebase already with compromises like this. I think we need to weigh 
> the performance gains we can get against the drawback in the cleanliness of 
> the abstraction. We can put a hold on this review and keep the methods public 
> for now if you are thinking about using them to improve performance and 
> conducting benchmarks to compare. :)
> 
> What are your thoughts @BenM?
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, yes, the `inline` works for this.
> 
> For keeping `add()/subtract()`, agree that we need a benchmark test for 
> this. The current 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274
>  was only used by quota and there is no benchmark test related for quota now, 
> perhaps we need to add one such case for benchmark test, @Ben and @Jiang Yan, 
> what do you think?
> 
> Another point is from 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1283-L1295
>  , if we have thousandes of agents and some quotas, I think the performance 
> will improve much based on the test result of 
> https://reviews.apache.org/r/50557/
> 
> Guangya Liu wrote:
> Had some offline discussion with Ben, we will make those two methods 
> add/subtract as private for now as it only impacts quota related logic in 
> allocator. And we can add some other wrapper functions to expose those two 
> APIs if they are needed in future. Also the benchmark test for `Quota` is not 
> priority, so there is no need to add them for now.
> 
> Jiang Yan Xu wrote:
> Alright I'll commit this then. So you haven't found other places on the 
> critical path that loops through a Resources object and add 

Re: Review Request 50836: Made add/subtract resource object as private method.

[Jiang Yan Xu]

> On Aug. 16, 2016, 4:44 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 59
> > 
> >
> > I think it would still work if you keep the `static` keyword right?
> > 
> > The `friend` keyword wouldn't allow you to specify `static` but here 
> > you can. 
> > 
> > Then in the cpp files you don't have to remove the `static` keyword on 
> > the definition.
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, but it still does not work as your proposal by adding 
> `static` for the `convertJSON` definition in cpp file and forward 
> declarations in hpp file as the `friend Try 
> internal::convertJSON()` will still think this method is not a `static` 
> method and this will cause build error as following.
> 
> ```
> In file included from ../../src/common/resources_utils.cpp:21:
> In file included from ../../src/common/resources_utils.hpp:21:
> ../../include/mesos/resources.hpp:59:25: error: unused function 
> 'convertJSON' [-Werror,-Wunused-function]
>   static Try convertJSON(
> ^
> 1 error generated.
> make[2]: *** [common/libmesos_no_3rdparty_la-resources_utils.lo] Error 1
> ```
> 
> Another is that I have posted some comments in 
> https://reviews.apache.org/r/50568/ and proposing keep `add` as public, can 
> you please take a look and show your comments if any.
> 
> The following are comments cited from https://reviews.apache.org/r/50568/
> 
> ```
> @Ben and @Yan, I found another place where we can enhance for allocator 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274
>  and this was only used by quota now, I think we can use add here. The 
> problem is that @Yan porposed to make the add as a private method and I've 
> posted a patch here https://reviews.apache.org/r/50836/diff/1#index_header , 
> but after a second thought, perhaps we can enable framework developers or 
> other users call this add API directly. As sometimes, the user may want to 
> traverse all Resources object and make some update for each Resource object 
> and then add those Resource again just like what we did in allocator for now. 
> So what about keep add as a public method?
> ```
> 
> Jiang Yan Xu wrote:
> My apologies. Looks like it's problematic to declare methods as static in 
> the header anyways. To keep the method internal to resources.cpp you can 
> define it as `inline` and keep the forward and friend declarations without 
> `inline` or `static`.
> 
> On the subject of keeping `add()/subtract()` public so some external code 
> can call them, I feel that this is a limitation of the current 
> Resource-related abstractions. If we keep the method public, we have to count 
> on the callers to know a lot about the internal implementation of `Resources` 
> and use it carefully. This is in general against information hiding and loose 
> coupling. 
> 
> Ideally if we have a C++ `Resource` abstraction that handles validation 
> and we know that a (non-proto) `Resource` object must be valid, we can make 
> `+=` validation free but until then, I understand that there are places in 
> Mesos codebase already with compromises like this. I think we need to weigh 
> the performance gains we can get against the drawback in the cleanliness of 
> the abstraction. We can put a hold on this review and keep the methods public 
> for now if you are thinking about using them to improve performance and 
> conducting benchmarks to compare. :)
> 
> What are your thoughts @BenM?
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, yes, the `inline` works for this.
> 
> For keeping `add()/subtract()`, agree that we need a benchmark test for 
> this. The current 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274
>  was only used by quota and there is no benchmark test related for quota now, 
> perhaps we need to add one such case for benchmark test, @Ben and @Jiang Yan, 
> what do you think?
> 
> Another point is from 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1283-L1295
>  , if we have thousandes of agents and some quotas, I think the performance 
> will improve much based on the test result of 
> https://reviews.apache.org/r/50557/
> 
> Guangya Liu wrote:
> Had some offline discussion with Ben, we will make those two methods 
> add/subtract as private for now as it only impacts quota related logic in 
> allocator. And we can add some other wrapper functions to expose those two 
> APIs if they are needed in future. Also the benchmark test for `Quota` is not 
> priority, so there is no need to add them for now.

Alright I'll commit this then. So you haven't found other places on the 
critical path that loops through a Resources object and add to another 
Resources object?

Re: Review Request 50836: Made add/subtract resource object as private method.

[Guangya Liu]

> On 八月 16, 2016, 11:44 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 59
> > 
> >
> > I think it would still work if you keep the `static` keyword right?
> > 
> > The `friend` keyword wouldn't allow you to specify `static` but here 
> > you can. 
> > 
> > Then in the cpp files you don't have to remove the `static` keyword on 
> > the definition.
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, but it still does not work as your proposal by adding 
> `static` for the `convertJSON` definition in cpp file and forward 
> declarations in hpp file as the `friend Try 
> internal::convertJSON()` will still think this method is not a `static` 
> method and this will cause build error as following.
> 
> ```
> In file included from ../../src/common/resources_utils.cpp:21:
> In file included from ../../src/common/resources_utils.hpp:21:
> ../../include/mesos/resources.hpp:59:25: error: unused function 
> 'convertJSON' [-Werror,-Wunused-function]
>   static Try convertJSON(
> ^
> 1 error generated.
> make[2]: *** [common/libmesos_no_3rdparty_la-resources_utils.lo] Error 1
> ```
> 
> Another is that I have posted some comments in 
> https://reviews.apache.org/r/50568/ and proposing keep `add` as public, can 
> you please take a look and show your comments if any.
> 
> The following are comments cited from https://reviews.apache.org/r/50568/
> 
> ```
> @Ben and @Yan, I found another place where we can enhance for allocator 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274
>  and this was only used by quota now, I think we can use add here. The 
> problem is that @Yan porposed to make the add as a private method and I've 
> posted a patch here https://reviews.apache.org/r/50836/diff/1#index_header , 
> but after a second thought, perhaps we can enable framework developers or 
> other users call this add API directly. As sometimes, the user may want to 
> traverse all Resources object and make some update for each Resource object 
> and then add those Resource again just like what we did in allocator for now. 
> So what about keep add as a public method?
> ```
> 
> Jiang Yan Xu wrote:
> My apologies. Looks like it's problematic to declare methods as static in 
> the header anyways. To keep the method internal to resources.cpp you can 
> define it as `inline` and keep the forward and friend declarations without 
> `inline` or `static`.
> 
> On the subject of keeping `add()/subtract()` public so some external code 
> can call them, I feel that this is a limitation of the current 
> Resource-related abstractions. If we keep the method public, we have to count 
> on the callers to know a lot about the internal implementation of `Resources` 
> and use it carefully. This is in general against information hiding and loose 
> coupling. 
> 
> Ideally if we have a C++ `Resource` abstraction that handles validation 
> and we know that a (non-proto) `Resource` object must be valid, we can make 
> `+=` validation free but until then, I understand that there are places in 
> Mesos codebase already with compromises like this. I think we need to weigh 
> the performance gains we can get against the drawback in the cleanliness of 
> the abstraction. We can put a hold on this review and keep the methods public 
> for now if you are thinking about using them to improve performance and 
> conducting benchmarks to compare. :)
> 
> What are your thoughts @BenM?
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, yes, the `inline` works for this.
> 
> For keeping `add()/subtract()`, agree that we need a benchmark test for 
> this. The current 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274
>  was only used by quota and there is no benchmark test related for quota now, 
> perhaps we need to add one such case for benchmark test, @Ben and @Jiang Yan, 
> what do you think?
> 
> Another point is from 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1283-L1295
>  , if we have thousandes of agents and some quotas, I think the performance 
> will improve much based on the test result of 
> https://reviews.apache.org/r/50557/

Had some offline discussion with Ben, we will make those two methods 
add/subtract as private for now as it only impacts quota related logic in 
allocator. And we can add some other wrapper functions to expose those two APIs 
if they are needed in future. Also the benchmark test for `Quota` is not 
priority, so there is no need to add them for now.


- Guangya


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

Re: Review Request 50836: Made add/subtract resource object as private method.

[Guangya Liu]

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

(Updated 八月 23, 2016, 10:19 p.m.)


Review request for mesos, Benjamin Mahler and Jiang Yan Xu.


Repository: mesos


Description
---

We do not want people call add/subtract resoure object directly,
so should make those methods as private.


Diffs (updated)
-

  include/mesos/resources.hpp bdbe8ea03a50cd1361640bde13a2342909723fc5 
  include/mesos/v1/resources.hpp c05cb634c7a5add78da00cb84fc75d3472a341bc 
  src/common/resources.cpp 96b4c39507bdb9b88aca5c2178b88057a5fc1881 
  src/v1/resources.cpp 3cc7580d5567370530c53759713be05c369bf298 

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


Testing
---

make
make check

```
./bin/mesos-tests.sh --gtest_filter="ResourcesTest.*"
./bin/mesos-tests.sh --gtest_filter="SharedResourcesTest.*"
```


Thanks,

Guangya Liu

Re: Review Request 50836: Made add/subtract resource object as private method.

[Guangya Liu]

> On 八月 16, 2016, 11:44 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 59
> > 
> >
> > I think it would still work if you keep the `static` keyword right?
> > 
> > The `friend` keyword wouldn't allow you to specify `static` but here 
> > you can. 
> > 
> > Then in the cpp files you don't have to remove the `static` keyword on 
> > the definition.
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, but it still does not work as your proposal by adding 
> `static` for the `convertJSON` definition in cpp file and forward 
> declarations in hpp file as the `friend Try 
> internal::convertJSON()` will still think this method is not a `static` 
> method and this will cause build error as following.
> 
> ```
> In file included from ../../src/common/resources_utils.cpp:21:
> In file included from ../../src/common/resources_utils.hpp:21:
> ../../include/mesos/resources.hpp:59:25: error: unused function 
> 'convertJSON' [-Werror,-Wunused-function]
>   static Try convertJSON(
> ^
> 1 error generated.
> make[2]: *** [common/libmesos_no_3rdparty_la-resources_utils.lo] Error 1
> ```
> 
> Another is that I have posted some comments in 
> https://reviews.apache.org/r/50568/ and proposing keep `add` as public, can 
> you please take a look and show your comments if any.
> 
> The following are comments cited from https://reviews.apache.org/r/50568/
> 
> ```
> @Ben and @Yan, I found another place where we can enhance for allocator 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274
>  and this was only used by quota now, I think we can use add here. The 
> problem is that @Yan porposed to make the add as a private method and I've 
> posted a patch here https://reviews.apache.org/r/50836/diff/1#index_header , 
> but after a second thought, perhaps we can enable framework developers or 
> other users call this add API directly. As sometimes, the user may want to 
> traverse all Resources object and make some update for each Resource object 
> and then add those Resource again just like what we did in allocator for now. 
> So what about keep add as a public method?
> ```
> 
> Jiang Yan Xu wrote:
> My apologies. Looks like it's problematic to declare methods as static in 
> the header anyways. To keep the method internal to resources.cpp you can 
> define it as `inline` and keep the forward and friend declarations without 
> `inline` or `static`.
> 
> On the subject of keeping `add()/subtract()` public so some external code 
> can call them, I feel that this is a limitation of the current 
> Resource-related abstractions. If we keep the method public, we have to count 
> on the callers to know a lot about the internal implementation of `Resources` 
> and use it carefully. This is in general against information hiding and loose 
> coupling. 
> 
> Ideally if we have a C++ `Resource` abstraction that handles validation 
> and we know that a (non-proto) `Resource` object must be valid, we can make 
> `+=` validation free but until then, I understand that there are places in 
> Mesos codebase already with compromises like this. I think we need to weigh 
> the performance gains we can get against the drawback in the cleanliness of 
> the abstraction. We can put a hold on this review and keep the methods public 
> for now if you are thinking about using them to improve performance and 
> conducting benchmarks to compare. :)
> 
> What are your thoughts @BenM?

Thanks Jiang Yan, yes, the `inline` works for this.

For keeping `add()/subtract()`, agree that we need a benchmark test for this. 
The current 
https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274
 was only used by quota and there is no benchmark test related for quota now, 
perhaps we need to add one such case for benchmark test, @Ben and @Jiang Yan, 
what do you think?

Another point is from 
https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1283-L1295
 , if we have thousandes of agents and some quotas, I think the performance 
will improve much based on the test result of 
https://reviews.apache.org/r/50557/


- Guangya


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


On 八月 5, 2016, 7:31 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50836/
> ---
> 
> (Updated 八月 5, 2016, 7:31 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 

Re: Review Request 50836: Made add/subtract resource object as private method.

[Jiang Yan Xu]

> On Aug. 16, 2016, 4:44 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 59
> > 
> >
> > I think it would still work if you keep the `static` keyword right?
> > 
> > The `friend` keyword wouldn't allow you to specify `static` but here 
> > you can. 
> > 
> > Then in the cpp files you don't have to remove the `static` keyword on 
> > the definition.
> 
> Guangya Liu wrote:
> Thanks Jiang Yan, but it still does not work as your proposal by adding 
> `static` for the `convertJSON` definition in cpp file and forward 
> declarations in hpp file as the `friend Try 
> internal::convertJSON()` will still think this method is not a `static` 
> method and this will cause build error as following.
> 
> ```
> In file included from ../../src/common/resources_utils.cpp:21:
> In file included from ../../src/common/resources_utils.hpp:21:
> ../../include/mesos/resources.hpp:59:25: error: unused function 
> 'convertJSON' [-Werror,-Wunused-function]
>   static Try convertJSON(
> ^
> 1 error generated.
> make[2]: *** [common/libmesos_no_3rdparty_la-resources_utils.lo] Error 1
> ```
> 
> Another is that I have posted some comments in 
> https://reviews.apache.org/r/50568/ and proposing keep `add` as public, can 
> you please take a look and show your comments if any.
> 
> The following are comments cited from https://reviews.apache.org/r/50568/
> 
> ```
> @Ben and @Yan, I found another place where we can enhance for allocator 
> https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274
>  and this was only used by quota now, I think we can use add here. The 
> problem is that @Yan porposed to make the add as a private method and I've 
> posted a patch here https://reviews.apache.org/r/50836/diff/1#index_header , 
> but after a second thought, perhaps we can enable framework developers or 
> other users call this add API directly. As sometimes, the user may want to 
> traverse all Resources object and make some update for each Resource object 
> and then add those Resource again just like what we did in allocator for now. 
> So what about keep add as a public method?
> ```

My apologies. Looks like it's problematic to declare methods as static in the 
header anyways. To keep the method internal to resources.cpp you can define it 
as `inline` and keep the forward and friend declarations without `inline` or 
`static`.

On the subject of keeping `add()/subtract()` public so some external code can 
call them, I feel that this is a limitation of the current Resource-related 
abstractions. If we keep the method public, we have to count on the callers to 
know a lot about the internal implementation of `Resources` and use it 
carefully. This is in general against information hiding and loose coupling. 

Ideally if we have a C++ `Resource` abstraction that handles validation and we 
know that a (non-proto) `Resource` object must be valid, we can make `+=` 
validation free but until then, I understand that there are places in Mesos 
codebase already with compromises like this. I think we need to weigh the 
performance gains we can get against the drawback in the cleanliness of the 
abstraction. We can put a hold on this review and keep the methods public for 
now if you are thinking about using them to improve performance and conducting 
benchmarks to compare. :)

What are your thoughts @BenM?


- Jiang Yan


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


On Aug. 5, 2016, 12:31 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50836/
> ---
> 
> (Updated Aug. 5, 2016, 12:31 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not want people call add/subtract resoure object directly,
> so should make those methods as private.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp bdbe8ea03a50cd1361640bde13a2342909723fc5 
>   include/mesos/v1/resources.hpp c05cb634c7a5add78da00cb84fc75d3472a341bc 
>   src/common/resources.cpp 2470c0280db7d48d9484c42bc2150e53e7ce6e1c 
>   src/v1/resources.cpp 6c4e3b299e701d477947dd7427c31d2ae64c05ae 
> 
> Diff: https://reviews.apache.org/r/50836/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh --gtest_filter="ResourcesTest.*"
> ./bin/mesos-tests.sh --gtest_filter="SharedResourcesTest.*"
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 50836: Made add/subtract resource object as private method.

[Guangya Liu]

> On 八月 16, 2016, 11:44 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 59
> > 
> >
> > I think it would still work if you keep the `static` keyword right?
> > 
> > The `friend` keyword wouldn't allow you to specify `static` but here 
> > you can. 
> > 
> > Then in the cpp files you don't have to remove the `static` keyword on 
> > the definition.

Thanks Jiang Yan, but it still does not work as your proposal by adding 
`static` for the `convertJSON` definition in cpp file and forward declarations 
in hpp file as the `friend Try internal::convertJSON()` will still 
think this method is not a `static` method and this will cause build error as 
following.

```
In file included from ../../src/common/resources_utils.cpp:21:
In file included from ../../src/common/resources_utils.hpp:21:
../../include/mesos/resources.hpp:59:25: error: unused function 'convertJSON' 
[-Werror,-Wunused-function]
  static Try convertJSON(
^
1 error generated.
make[2]: *** [common/libmesos_no_3rdparty_la-resources_utils.lo] Error 1
```

Another is that I have posted some comments in 
https://reviews.apache.org/r/50568/ and proposing keep `add` as public, can you 
please take a look and show your comments if any.

The following are comments cited from https://reviews.apache.org/r/50568/

```
@Ben and @Yan, I found another place where we can enhance for allocator 
https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1274
 and this was only used by quota now, I think we can use add here. The problem 
is that @Yan porposed to make the add as a private method and I've posted a 
patch here https://reviews.apache.org/r/50836/diff/1#index_header , but after a 
second thought, perhaps we can enable framework developers or other users call 
this add API directly. As sometimes, the user may want to traverse all 
Resources object and make some update for each Resource object and then add 
those Resource again just like what we did in allocator for now. So what about 
keep add as a public method?
```


- Guangya


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


On 八月 5, 2016, 7:31 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50836/
> ---
> 
> (Updated 八月 5, 2016, 7:31 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not want people call add/subtract resoure object directly,
> so should make those methods as private.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp bdbe8ea03a50cd1361640bde13a2342909723fc5 
>   include/mesos/v1/resources.hpp c05cb634c7a5add78da00cb84fc75d3472a341bc 
>   src/common/resources.cpp 2470c0280db7d48d9484c42bc2150e53e7ce6e1c 
>   src/v1/resources.cpp 6c4e3b299e701d477947dd7427c31d2ae64c05ae 
> 
> Diff: https://reviews.apache.org/r/50836/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh --gtest_filter="ResourcesTest.*"
> ./bin/mesos-tests.sh --gtest_filter="SharedResourcesTest.*"
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 50836: Made add/subtract resource object as private method.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50836]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Aug. 5, 2016, 7:31 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50836/
> ---
> 
> (Updated Aug. 5, 2016, 7:31 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not want people call add/subtract resoure object directly,
> so should make those methods as private.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp bdbe8ea03a50cd1361640bde13a2342909723fc5 
>   include/mesos/v1/resources.hpp c05cb634c7a5add78da00cb84fc75d3472a341bc 
>   src/common/resources.cpp 2470c0280db7d48d9484c42bc2150e53e7ce6e1c 
>   src/v1/resources.cpp 6c4e3b299e701d477947dd7427c31d2ae64c05ae 
> 
> Diff: https://reviews.apache.org/r/50836/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh --gtest_filter="ResourcesTest.*"
> ./bin/mesos-tests.sh --gtest_filter="SharedResourcesTest.*"
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 50836: Made add/subtract resource object as private method.

[Guangya Liu]

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

Review request for mesos, Benjamin Mahler and Jiang Yan Xu.


Repository: mesos


Description
---

We do not want people call add/subtract resoure object directly,
so should make those methods as private.


Diffs
-

  include/mesos/resources.hpp bdbe8ea03a50cd1361640bde13a2342909723fc5 
  include/mesos/v1/resources.hpp c05cb634c7a5add78da00cb84fc75d3472a341bc 
  src/common/resources.cpp 2470c0280db7d48d9484c42bc2150e53e7ce6e1c 
  src/v1/resources.cpp 6c4e3b299e701d477947dd7427c31d2ae64c05ae 

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


Testing
---

make
make check

```
./bin/mesos-tests.sh --gtest_filter="ResourcesTest.*"
./bin/mesos-tests.sh --gtest_filter="SharedResourcesTest.*"
```


Thanks,

Guangya Liu