> On Aug. 16, 2016, 4: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. > > 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<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? > ```
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 > >