mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Guangya Liu <gyliu...@gmail.com>
Subject Re: Review Request 50836: Made add/subtract resource object as private method.
Date Wed, 17 Aug 2016 00:06:03 GMT


> On 八月 16, 2016, 11:44 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 59
> > <https://reviews.apache.org/r/50836/diff/1/?file=1464015#file1464015line59>
> >
> >     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<Resources>
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<Resources> 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
> 
>


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