mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 50836: Made add/subtract resource object as private method.
Date Tue, 06 Sep 2016 21:14:27 GMT


> 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?
>     ```
> 
> 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?
> 
> Guangya Liu wrote:
>     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());
>     ```

Ahh I didn't look at this closely enough. So the whole loop was only for stripping out the
role, i.e., modifying the Resource object before adding to another Resources. So it seems
`filter()` is not applicable here. We could instead have a `transform()` method that does
what we intended here. I'll post a review for you.


- Jiang Yan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50836/#review145918
-----------------------------------------------------------


On Aug. 23, 2016, 3:19 p.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50836/
> -----------------------------------------------------------
> 
> (Updated Aug. 23, 2016, 3: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
> 
>


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