> On July 28, 2016, 6:14 p.m., Jiang Yan Xu wrote: > > src/tests/resources_tests.cpp, line 2727 > > <https://reviews.apache.org/r/45959/diff/9/?file=1448698#file1448698line2727> > > > > 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.
I was suggesting killing the `Contains` test (in addition to adding the `Count`) because it's a bit complicated IMO and the things it tested are covered in other tests. Will not include `SharedResourcesTest.Contains` for now, we can chat about adding it back if necessary in a later patch. - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45959/#review143997 ----------------------------------------------------------- On July 29, 2016, 11:59 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45959/ > ----------------------------------------------------------- > > (Updated July 29, 2016, 11:59 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 6638c8f79e45c0ff3efa4342e30478d2e1e4740b > src/common/resources.cpp 468581da550bcabf44fbaba8897d5fbbc330c2cb > 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 > >
