----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45959/#review141176 -----------------------------------------------------------
Code LGTM. :) Comments mainly on tests. include/mesos/resources.hpp (line 72) <https://reviews.apache.org/r/45959/#comment206606> For implicit converting constructors it's the convention to add an "annotation". ``` /*implicit*/ Resource_(const Resource& _resource) ``` include/mesos/resources.hpp (line 99) <https://reviews.apache.org/r/45959/#comment206595> s/adjusted/adjusted or compared/ ? src/common/resources.cpp (line 856) <https://reviews.apache.org/r/45959/#comment206596> Align the two like this: ``` return sharedCount.get() >= that.sharedCount.get() && resource == that.resource; ``` src/common/resources.cpp (lines 894 - 898) <https://reviews.apache.org/r/45959/#comment206597> I commented on this previously: If the substraction results in empty or invalid `Resource_`, it's `Resources::operator-=`'s responsibility to clean it up. In other words, It's perfectly fine to have "cpus:100<0>" or "cpus:100<-1>" by themselves the same way it's fine to have "cpus:-1" by itself. It's `Resources` that doesn't allow them in it. Therefore we can remove this from here. src/common/resources.cpp (line 909) <https://reviews.apache.org/r/45959/#comment206601> Not *contained* here. How about something like *cannot be equal*? src/common/resources.cpp (line 914) <https://reviews.apache.org/r/45959/#comment206602> s/a shared resource/shared resources/ s/of these resources// (because of redundancy) ? src/tests/mesos.hpp (lines 594 - 597) <https://reviews.apache.org/r/45959/#comment206613> Put this in the if block above? src/tests/resources_tests.cpp (line 2457) <https://reviews.apache.org/r/45959/#comment206688> A general comment is that I see this test (and the one above) mirrors `ScalarSubtraction` (and `ScalarAddition`) but the code duplication is unfortunate. Some duplication is justified beacuse we are verifying nonshared resource arithmetic operations still work in `Resources` when shared resources are present. However let's try to minimize the duplication and length the these tests. src/tests/resources_tests.cpp (lines 2459 - 2493) <https://reviews.apache.org/r/45959/#comment206692> The test tends to get pretty long due to these constructions. Utimately it looks like we are just constructing two `Resources` `r1` and `r2` and testing their `diff`. How about simply: ``` Resource disk = createDiskResource("8192", "role1", "1", "path", None(), true); Resource r1 = Resources::parse("cpus:40;mem:4096").get() + disk + disk; Resource r2 = Resources::parse("cpus:5;mem:512").get() + disk; Resources diff1 = r1 - r2; ``` Note that we are focusing on subtraction here. src/tests/resources_tests.cpp (lines 2468 - 2474) <https://reviews.apache.org/r/45959/#comment206689> Why add them up since we are testing substraction? Can we construct r1 directly? src/tests/resources_tests.cpp (lines 2502 - 2507) <https://reviews.apache.org/r/45959/#comment206651> Let's not reuse `diff1` (it's compound assignment and not a `diff` anymore). If we want to verify that `-=` is equivalent to `-` we can: ``` Resources r = r1; r1 -= r2; // The same expectations as those of diff1. ``` src/tests/resources_tests.cpp (lines 2509 - 2524) <https://reviews.apache.org/r/45959/#comment206694> So what do these line do? Looks like we are interested in testing the situation where a shared resource is fully removed from `Resources` after substraction. So let's add a comment to state the goal and test it explicitly. Also after verifying basic `-` and `-=` already we can keep the additional expecatations concise. How about: ``` // Verify that the shared disk is removed from Resources after all copies are substracted. // r1 has two copies of 'disk' and r2 has one. EXPECT_EQ(0, (r1 - r2 - r2).count(disk)); EXPECT_EQ(0, (r2 - r1).count(disk)); ``` src/tests/resources_tests.cpp (line 2528) <https://reviews.apache.org/r/45959/#comment206638> The test is a bit long and difficult to follow and it seems to cover a lot of things already covered in the two tests above. About `MultipleConsumers`: the tests above already use Resources objects with multiple copies of the same shared resources so we don't need redo it here. I suggest for this test we focus on more complex expressions such as `EXPECT_EQ(r1 + r1 - r1, r1)` and additional methods such as `contains()`. In order to shorten and simplify the test: - Inline the variable constructions wherever possible. - Don't reuse and modify variables like `sum` and `diff` as the test progresses as it's hard to follow their changed meanings. - Don't use `get<Value::Scalar>` to verify each single resource in a Resources object as we have already done it in the above two tests. Use exepctations to directly compare Resources objects. src/tests/resources_tests.cpp (lines 2559 - 2561) <https://reviews.apache.org/r/45959/#comment206633> Space around operators like this: ``` EXPECT_EQ(r1 + r1 - r1, r1); ``` How about adding one more: ``` EXPECT_EQ(r2 + r1 - r1, r2); ``` src/tests/resources_tests.cpp (line 2563) <https://reviews.apache.org/r/45959/#comment206639> I suggest we don't reuse variables, perhaps even use `const`. ``` const Resources sum = r1 + r2; const Resources diff = r1 - r2; ``` In the following lines it becomes unclear what each variable means at a given moment anymore... src/tests/resources_tests.cpp (line 2591) <https://reviews.apache.org/r/45959/#comment206640> To test the "contain relationship with two resoures differ by only the count of one shared resource" I would just do: ``` EXPECT_TRUE(r1.contains(r1 - disk1)); EXPECT_FALSE((r1 - disk1).contains(r1)); ``` Also add a comment. src/tests/resources_tests.cpp (line 2599) <https://reviews.apache.org/r/45959/#comment206751> How about subtractions and contains? Seems like ScalarNonEqualSharedResources and ScalarSharedNonSharedOperations should be pretty much the same with the only difference being `isShared = true` in one `createDiskResource` call? src/tests/resources_tests.cpp (line 2624) <https://reviews.apache.org/r/45959/#comment206697> The test name is not very descriptive so we should add comments to describe the test.\ Call it `ScalarSharedAndNonSharedResources` (following the naming pattern of the previous test)? src/tests/resources_tests.cpp (lines 2626 - 2649) <https://reviews.apache.org/r/45959/#comment206698> We can shorten this to: ``` Resources r1 = Resources::parse("cpus:1;mem:5).get() + disk1; Resources r2 = Resources::parse("cpus:1;mem:5).get() + disk2; EXPECT_FALSE(r2.contains(r1)); EXPECT_FALSE(r1.contains(r2)); ``` src/tests/resources_tests.cpp (line 2651) <https://reviews.apache.org/r/45959/#comment206753> s/sum/r/ when in the compound assignment case. src/tests/resources_tests.cpp (lines 2661 - 2664) <https://reviews.apache.org/r/45959/#comment206752> s/diff/r/ when using the compound assignment form. Or, ``` EXPECT_EQ((Resources(disk2) - disk1, disk2); EXPECT_EQ(Resources(disk1) - disk2, disk1); ``` - Jiang Yan Xu On June 13, 2016, 12:18 a.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45959/ > ----------------------------------------------------------- > > (Updated June 13, 2016, 12:18 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. > > Note that v1 changes for shared resources are in the next commit. > > > Diffs > ----- > > include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 > src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 > src/master/validation.cpp 7b9c2281b2ab1295211f0dd385cd77947fbd63be > src/tests/mesos.hpp e9361a65eb31cced99e9ed32fd18a65d1bda26e3 > src/tests/resources_tests.cpp dc12bd8f1e2da6972bc8aed598811c55d664036e > > 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 > >
