> 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? > > 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 ----------------------------------------------------------- On 八月 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 八月 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 > >
