mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anindya Sinha <anindya_si...@apple.com>
Subject Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.
Date Mon, 13 Jun 2016 07:18:34 GMT


> On June 10, 2016, 12:10 a.m., Jiang Yan Xu wrote:
> > I feel good about the overall structure of this change with this revision. I hope
I have captured most things so if we do another pass after it should mostly be relatively
minor readability/reusablity/documentation/cleanups etc. which is hard to do in one go due
to the length. 
> > 
> > ## About v1 resources files
> > In retropsect I feel like it probably works the better if it's in a separate patch
done **after the v0 files are shipped** to avoid having them go through all intermediate revisions.
i.e., you can update v1 files according to the final version of the v0 files.
> > 
> > I have mostly skipped v1 files in this review because the comments would have been
duplicated. Since we are still iterating on this, would you mind pulling them out from this
review?
> > 
> > ## Validation
> > If we currently only allow persistent volumes to be shared, should we validate this
here?
> > 
> > ## Additional test ideas
> > - Resources A + A - A == A with shared resources in it. 
> > - Resources B + A - A == B with shared resources in A (empty resources are discarded)
> > - Resources r1 = A + A, r2 = A + A + A, r1 - r2 is empty (invalid entries are discarded)
> > - Resources r1 + A == r1 (A is an invalid shared resource) (invalid entries are
discarded)
> > - Shared resources that slightly differ are not grouped together.
> > - Shared resources stream output.
> > - Resources contains() with shared resources.
> > - Any existing resources tests interesting when shared resources are blended in?
> 
> Jiang Yan Xu wrote:
>     About validation: sorry I didn't realize it's in a later review /r/45960/ but if
you just look at this review one might wonder if you should also test shared cpus or other
fungible resources. Anyways, it's fine if we don't since it's not a valid use case for now.

Moved v1 changes to https://reviews.apache.org/r/48616/, and added more tests.


> On June 10, 2016, 12:10 a.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 64
> > <https://reviews.apache.org/r/45959/diff/4/?file=1390634#file1390634line64>
> >
> >     We have a private section below already. Put `Resource_` definition there?

`Resource_` needs to be defined before it is used. Since we use it in the public section,
it is defined prior to that and hence the additional `private` section in order to avoid moving
the original `private` section of `Resources`.


> On June 10, 2016, 12:10 a.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 454-455
> > <https://reviews.apache.org/r/45959/diff/4/?file=1390634#file1390634line454>
> >
> >     Doesn't `Resource_` already friend the << operator?

This is because `Resource_` is a private member of `mesos::Resources`. Note this non-member
global function has `const Resources::Resource_& resource` as its argument.


> On June 10, 2016, 12:10 a.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, line 864
> > <https://reviews.apache.org/r/45959/diff/4/?file=1390636#file1390636line864>
> >
> >     This comment still has "contained" in it.
> >     
> >     How about:
> >     
> >     ```
> >     // The sharedness needs to match for two equal Resources_ objects.
> >     ```

Updated the comment as:
`// Both Resource_ objects need to shared resources, or both need to be non-shared resources.`


> On June 10, 2016, 12:10 a.m., Jiang Yan Xu wrote:
> > src/tests/resources_tests.cpp, line 2534
> > <https://reviews.apache.org/r/45959/diff/4/?file=1390639#file1390639line2534>
> >
> >     How is this test different than `ScalarAdditionShared` test?
> >     
> >     `ScalarAdditionShared` also has multiple copies of the same shared disk added
together which results in `EXPECT_EQ(2, r.count(disk1));`.

Modified `ScalarMultipleConsumers` such that it exhibits arithmetic operations involving shared
count > 1. `ScalarAdditionShared` deals with adding with shared count of 1.


> On June 10, 2016, 12:10 a.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 86-87
> > <https://reviews.apache.org/r/45959/diff/4/?file=1390634#file1390634line86>
> >
> >     There's a pattern worth explaining so we can put `contains` together with the
operators and have a common comment block.
> >     
> >     ```
> >     The Resource_ arithmetic operators and equality/contains comparison require
the wrapped protobuf `resource` to have the same sharedness. 
> >     
> >     For shared resources, their protobuf `resource` fields need to be the same (operators
and comparisons apply to the counters). Otherwise the result is as though the arithmetic operators
are not applied or 'false' for equality/contains comparison.
> >     
> >     For non-shared resources the counters are none and the semantics of the Resource_
operators/comparison are the same as Resource's. See comments above the Resource operators.
> >     ```
> >     
> >     Then I feel the comments for the operators and contains in `Resources_` are
not necessary anymore. I feel either the comment is too trivial or the comments are too duplicated
if on individual operators/methods. What do you think?

Consolidated the comments for these functions.


- Anindya


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


On May 22, 2016, 7:08 a.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> -----------------------------------------------------------
> 
> (Updated May 22, 2016, 7:08 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joris Van Remoortere, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4892
>     https://issues.apache.org/jira/browse/MESOS-4892
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> A new class Resource_ is added that allows 'Resources' to group
> identical shared resource objects together into a single 'Resource_'
> object and tracked by its shared count. Non-shared resource objects
> are not grouped.
> 
> For resource addition and subtraction, the shared count is adjusted for
> shared resources as follows:
> a) Addition: If shared resource is absent from original, then the
>    resource is added initialized with a consumer count of 1. Otherwise,
>    the share count for the shared resource is incremented.
> b) Subtraction: If shared resource's share count is already 1, then
>    the shared resource is removed from the original. Otherwise, its
>    consumer count is decremented.
> 
> 
> Diffs
> -----
> 
>   include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
>   include/mesos/v1/resources.hpp a5ba8fec4c9c3643646308f75a4b28cefe0b3df3 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/master/validation.cpp f490b899758bdac9676a6f6939918efa6ac52781 
>   src/tests/mesos.hpp 79bf1ff16412ce2a510a9b75ab1ac91c1c182653 
>   src/tests/resources_tests.cpp dc12bd8f1e2da6972bc8aed598811c55d664036e 
>   src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 
> 
> Diff: https://reviews.apache.org/r/45959/diff/
> 
> 
> Testing
> -------
> 
> New tests added to demonstrate arithmetic operations for shared resources with consumer
counts.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>


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