----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45959/#review136417 -----------------------------------------------------------
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? include/mesos/resources.hpp (line 64) <https://reviews.apache.org/r/45959/#comment201981> We have a private section below already. Put `Resource_` definition there? include/mesos/resources.hpp (line 76) <https://reviews.apache.org/r/45959/#comment201459> Instead of stating what the line does literally, add a bit of explanation: // Setting the counter to 1 to indicate "one copy" of the shared resource. include/mesos/resources.hpp (lines 86 - 87) <https://reviews.apache.org/r/45959/#comment202047> 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? include/mesos/resources.hpp (line 116) <https://reviews.apache.org/r/45959/#comment201473> s/The share count for the protobuf Resource object./The counter for grouping shared 'resource' objects, None if 'resource' is non-shared./ include/mesos/resources.hpp (line 121) <https://reviews.apache.org/r/45959/#comment202049> Since we friend the outter class. Make data members private? include/mesos/resources.hpp (lines 125 - 126) <https://reviews.apache.org/r/45959/#comment201474> One blank line here. include/mesos/resources.hpp (lines 283 - 285) <https://reviews.apache.org/r/45959/#comment201945> Can we avoid explaining sharedCount or share count in the public method because it's an internal detail? ``` Count the Resource objects that match the specified value. NOTE: - For a non-shared resource the count can be at most 1 because all non-shared Resource objects in Resources are unique. - For a shared resource the count can be greater than 1. ``` include/mesos/resources.hpp (lines 449 - 450) <https://reviews.apache.org/r/45959/#comment201854> Doesn't `Resource_` already friend the << operator? include/mesos/resources.hpp (lines 451 - 452) <https://reviews.apache.org/r/45959/#comment201855> Why this `friend`? This operator is already defined and we are not changing it right? src/common/resources.cpp (line 826) <https://reviews.apache.org/r/45959/#comment201902> How about "Invalid shared resource: count < 0"? src/common/resources.cpp (line 843) <https://reviews.apache.org/r/45959/#comment201903> So if (this, that) = (cpus:5<3>, cpus:4<2>), should `this` contain `that`? It probably shouldn't? ``` // The sharedness needs to match for two Resource_ // objects to be contained within one another. if (isShared() != that.isShared()) { return false; } if (isShared()) { return sharedCount.get() >= that.sharedCount.get() && resource == that.resource; } else { return internal::contains(resource, that.resource); } ``` src/common/resources.cpp (line 864) <https://reviews.apache.org/r/45959/#comment201904> This comment still has "contained" in it. How about: ``` // The sharedness needs to match for two equal Resources_ objects. ``` src/common/resources.cpp (lines 887 - 895) <https://reviews.apache.org/r/45959/#comment201917> If both objects are non-shared, we still has to check is their `resource` fields are addable right? How about the following? I pulled the "update sharedCount" comment up to explain the CHECKs. ``` if (internal::addable(resource, that.resource)) { if (!isShared() && !that.isShared()) { resource += that.resource; } else { // 'addable' makes sure both 'resource' fields are shared and // equal so we just need to sum up the counters here. CHECK_SOME(sharedCount); CHECK_SOME(that.sharedCount); sharedCount = sharedCount.get() + that.sharedCount.get(); } } ``` src/common/resources.cpp (line 903) <https://reviews.apache.org/r/45959/#comment201919> This can be updated according to the comments for `operator+=` above. Besides, why the need for `isRemove` here? If the substraction results in empty or invalid `Resource_`, it's `Resources::operator-=`'s responsibility to clean it up. src/common/resources.cpp (line 905) <https://reviews.apache.org/r/45959/#comment201918> We don't need the extra `()`. src/common/resources.cpp (line 986) <https://reviews.apache.org/r/45959/#comment201959> A blank line above it. src/tests/mesos.hpp (line 580) <https://reviews.apache.org/r/45959/#comment201984> Don't we just need a `bool isShared`? src/tests/resources_tests.cpp (lines 181 - 182) <https://reviews.apache.org/r/45959/#comment201995> We have already casted it into `const Resource&`, using a pointer to take its address just so it can use `->` is a bit odd. After all `cpus` was previously an iterator. This results in fewer lines changed but it's harder to understand why this is done. In this case I think changing the following lines from using `->` to using `.` is totally reasonable. e.g., ``` ASSERT_EQ(Value::SCALAR, cpus.type()); ``` Here and below. src/tests/resources_tests.cpp (line 2506) <https://reviews.apache.org/r/45959/#comment201997> Kill line. src/tests/resources_tests.cpp (line 2534) <https://reviews.apache.org/r/45959/#comment201998> 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));`. - Jiang Yan Xu On May 22, 2016, 12: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, 12: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 > >
