----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45959/#review143997 -----------------------------------------------------------
This is done before the you rebased but it should be straightforward once we address these remaining issues and we should be good to go! include/mesos/resources.hpp (line 64) <https://reviews.apache.org/r/45959/#comment209961> Perhaps a short note: ``` // The rest of the private section is below the public section. We need to define Resource_ first because the public typedefs below depend on it. ``` include/mesos/resources.hpp (line 95) <https://reviews.apache.org/r/45959/#comment209936> Can we reword it as ``` The `Resource_` arithmetric and comparison operators and `contains()` method require... ``` include/mesos/resources.hpp (line 96) <https://reviews.apache.org/r/45959/#comment209934> I didn't raise this earlier but to maintain consistency in comments let's just do s/shared property/sharedness/ include/mesos/resources.hpp (line 101) <https://reviews.apache.org/r/45959/#comment209937> s/comparison/`contains()` method/ comparisions (i.e., ==, !=) are operators too. include/mesos/resources.hpp (line 471) <https://reviews.apache.org/r/45959/#comment209962> s/is/are/ src/common/resources.cpp (lines 255 - 279) <https://reviews.apache.org/r/45959/#comment209930> Why the change from the last revision? Sharedness check at the top allows us to short-circuit the checking for shared resources and allow us to not have to consider shared resources as a possbility in subsequent if conditions which is better IMO both for simplicity and performance (even if the difference in performance may not be significant). src/common/resources.cpp (lines 323 - 346) <https://reviews.apache.org/r/45959/#comment209963> Ditto. src/common/resources.cpp (lines 842 - 843) <https://reviews.apache.org/r/45959/#comment209969> Just trying to see if we can make the comments more concise. ``` // Same sharedness required. ``` src/common/resources.cpp (lines 848 - 850) <https://reviews.apache.org/r/45959/#comment209966> Just trying to see if the comments can be made more concise: ``` Assuming the wrapped Resource objects are equal, the 'contains' relationship is determined by the relationship of the counters for shared resources. ``` src/common/resources.cpp (line 856) <https://reviews.apache.org/r/45959/#comment209968> Here an `else` clause is more explicit. ``` else { return internal::contains(resource, that.resource); } ``` src/common/resources.cpp (lines 900 - 901) <https://reviews.apache.org/r/45959/#comment209970> Just trying to see if we can make the comments more concise. ``` // Same sharedness required. ``` src/common/resources.cpp (line 979) <https://reviews.apache.org/r/45959/#comment209973> Looks like we don't need `const Resource& resource = resource_;` and we can just do: ``` if (resource_.resource == that) { ... } ``` ? src/tests/resources_tests.cpp (line 2675) <https://reviews.apache.org/r/45959/#comment209974> In my previous comment I was suggesting that let's make sections of the test not depend on one another. i.e., r2 doesn't depend on r1. (Of course as a reader I still need to know the definition of disk1, disk2 and disk3). Speaking of which, disk3 and disk2 are the same so perhaps we don't need a disk3 (once we established in simple tests that the equality operator works). If it's written this way, it's then very clear to me what exactly we are testing in each section and whether some tests are redundant. e.g., ``` Resources r2 = Resources(disk1) + disk2; (Addition) ... Resources r3 = Resources(disk1) + disk2 - disk1; (Can substract) ... Resources r4 = Resources(disk1) - disk2; (Cannot substract) ``` src/tests/resources_tests.cpp (line 2700) <https://reviews.apache.org/r/45959/#comment209996> Seems like adding disk1 twice doesn't particularly help with anything? For symmetry maybe just ``` Resources r1 = Resources::parse("cpus:1;mem:5").get() + disk1; Resources r2 = Resources::parse("cpus:1;mem:5").get() + disk2; ``` src/tests/resources_tests.cpp (line 2702) <https://reviews.apache.org/r/45959/#comment209994> Kill this line. We don't have it for r2 and we it's already covered in other tests. src/tests/resources_tests.cpp (line 2707) <https://reviews.apache.org/r/45959/#comment209995> Simple heading. // r1 and r2 don't contain each other because of disk1 and disk2's different sharedness. src/tests/resources_tests.cpp (line 2710) <https://reviews.apache.org/r/45959/#comment209998> Simple heading. // Additions. src/tests/resources_tests.cpp (line 2720) <https://reviews.apache.org/r/45959/#comment209992> Simple headings. // Cannot substract. src/tests/resources_tests.cpp (line 2727) <https://reviews.apache.org/r/45959/#comment210007> I apologize for my typo in the previous review, I was going to suggest a `count` test because it's not covered by existing tests for non-shared resources and there are some interesting scenarios here. I was thinking of something like this. ``` TEST(ResourcesTest, Count) { Resource sharedDisk = createDiskResource( "100", "role1", "2", "path2", None(), true); EXPECT_EQ(1, (Resources(sharedDisk)).count(shared)); EXPECT_EQ(2, (Resources(sharedDisk) + shared).count(shared)); // The summation is invalid and a no-op for non-shared disks so the // count remains 1. Resource nonsharedDisk = createDiskResource("100", "role1", "2", "path2"); EXPECT_EQ(1, (Resources(nonsharedDisk)).count(nonsharedDisk)); EXPECT_EQ(1, (Resources(nonsharedDisk) + nonsharedDisk).count(nonsharedDisk)); // After the summation the scalar changes so the count is 0. Resources cpus = Resources::parse("cpus:1").get(); EXPECT_EQ(1, (cpus).count(cpus)); EXPECT_EQ(0, (cpus + cpus).count(cpus)); } ``` A dedicated `Contains` test would have been nice if it can simpify other tests and if there was one it should probably be no longer or more complex than the `Count` test. However given the way other tests are written perhaps don't stress about it. - Jiang Yan Xu On July 28, 2016, 3:42 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45959/ > ----------------------------------------------------------- > > (Updated July 28, 2016, 3:42 p.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 88a9feabf66ed34e7e5b1c6cb7e831818e7f7883 > src/common/resources.cpp 3dbff24d6859d3b1ed8589cec50170a5202cfbcb > src/master/validation.cpp 52002beac29c371411348cb026a216e99ac96ab2 > src/tests/mesos.hpp 51c66f175c80ebacd5af230222ea7e4c81dfc1e8 > src/tests/resources_tests.cpp 4111e080b84079e100b731c9a56861b204f17388 > > 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 > >
