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