> 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/
>
> 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 to another
> Resources object?
>
> Guangya Liu wrote:
> Yes, no other places except `quota` in `allocator` ;-)
>
> Jiang Yan Xu wrote:
> Hey actually, in the quota example you provided, can we leverage
> `Resources::filter()` to get the performance benefits from private add/remove?
Yes, that would help, the logic can be as following:
```
auto getQuotaRoleAllocatedResources = [this](const string& role) {
CHECK(quotas.contains(role));
// NOTE: `allocationScalarQuantities` omits dynamic reservation and
// persistent volume info, but we additionally strip `role` here.
Resources resources = quotaRoleSorter->allocationScalarQuantities(role);
resources.filter([](const Resource& resource) {
return !resource.has_disk() && !resource.has_reservation();
});
return resources.flatten();
};
```
The only problem is that do we still need to keep the `CHECK` here?
```
CHECK(!resource.has_reservation());
CHECK(!resource.has_disk());
```
- 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
>
>
