> 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 > >
