> On April 4, 2017, 11:16 p.m., Jie Yu wrote: > > src/tests/resources_tests.cpp > > Lines 1537 (patched) > > <https://reviews.apache.org/r/57998/diff/3/?file=1681846#file1681846line1537> > > > > Instead of just one type of resources, i'd try to use multiple types of > > resources here (e.g., "disk" and "cpu") so that we exercise the path that > > two different types of the resources might have the same resource provider > > ID.
I added some operations involving additional `cpus`. > On April 4, 2017, 11:16 p.m., Jie Yu wrote: > > src/tests/resources_tests.cpp > > Lines 1546 (patched) > > <https://reviews.apache.org/r/57998/diff/3/?file=1681846#file1681846line1546> > > > > I would also check r1's size here. Done. > On April 4, 2017, 11:16 p.m., Jie Yu wrote: > > src/tests/resources_tests.cpp > > Lines 1547 (patched) > > <https://reviews.apache.org/r/57998/diff/3/?file=1681846#file1681846line1547> > > > > I'd move this to the dedicated 'contains' test I intoduced a dedicated test for `contains`, but would prefer to leave this check in this test since I believe it captures a necessary postcondition of the kind of addition we are interested here. Suggest to drop this issue. > On April 4, 2017, 11:16 p.m., Jie Yu wrote: > > src/tests/resources_tests.cpp > > Lines 1548 (patched) > > <https://reviews.apache.org/r/57998/diff/3/?file=1681846#file1681846line1548> > > > > looks like this check is for shared resources. Can you remove this > > check from this test? Removed. > On April 4, 2017, 11:16 p.m., Jie Yu wrote: > > src/tests/resources_tests.cpp > > Lines 1559-1560 (patched) > > <https://reviews.apache.org/r/57998/diff/3/?file=1681846#file1681846line1559> > > > > Ditto. `count` sounds very shared resource related. I used `Resources::count` as a very simple way to examine the behavior of `addable`. If resources with different `provider_id`s where addable `count` would yield `0` in both cases, and some other disk would appear. The fact that both counts are `1` tells us that the resources were not added and that the result has both results as separate resources. Alternatively one could use e.g., `Resources::filter` to separate resources with that provider_id and without it; one could then compare these resources against `disk1` and `disk2`. I believe that code would be much lengthier, and not necessarily much clearer. Suggest to drop this issue. > On April 4, 2017, 11:16 p.m., Jie Yu wrote: > > src/tests/resources_tests.cpp > > Lines 1572 (patched) > > <https://reviews.apache.org/r/57998/diff/3/?file=1681846#file1681846line1572> > > > > Ditto. I would prefer we include another type of resources with the > > same provider ID here in this test. Added some operations involving `cpus` here. > On April 4, 2017, 11:16 p.m., Jie Yu wrote: > > src/tests/resources_tests.cpp > > Lines 1577-1578 (patched) > > <https://reviews.apache.org/r/57998/diff/3/?file=1681846#file1681846line1577> > > > > This is contains check, let's factor this out into a separate test. Removed here. > On April 4, 2017, 11:16 p.m., Jie Yu wrote: > > src/tests/resources_tests.cpp > > Lines 1594 (patched) > > <https://reviews.apache.org/r/57998/diff/3/?file=1681846#file1681846line1594> > > > > Let's also add some test around equality (i.e., checking if `==` or > > `!=` works properly or not. For instance, the fact that you don't have a > > `==` defined for unversioned API should be captured here in the unit tests. I added a dedicated check for (n)equality. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57998/#review171040 ----------------------------------------------------------- On April 5, 2017, 3:56 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57998/ > ----------------------------------------------------------- > > (Updated April 5, 2017, 3:56 p.m.) > > > Review request for mesos, Jie Yu and Jan Schlicht. > > > Bugs: MESOS-7312 > https://issues.apache.org/jira/browse/MESOS-7312 > > > Repository: mesos > > > Description > ------- > > This patch adds an optional resource provider id to resources. In > future changes we will introduce abstract providers of resources. > While currently agents are implicit resource providers, later on an > agent might use multiple resource providers. By having a provider id > in the resource we can unambigously detect which provider contributed > which resource. > > > Diffs > ----- > > include/mesos/mesos.proto dd90465cc3da283c078d4e907cc6a4a0e50309ac > include/mesos/v1/mesos.proto 228623155c7f68c0f24d173aacbc6eb734f1382f > src/common/resources.cpp c26e0f995006dc6b2e70a491cea58fa90347e42a > src/tests/resources_tests.cpp 343cab2af225a05e32c5a8bd4a5d9ddfbf76536d > src/tests/resources_utils.cpp 2cef55f7312d671307e097c2c4960c8dcf45c1ff > src/v1/resources.cpp a53deafbea399a1bcf729d1c151bc46e9da04e11 > > > Diff: https://reviews.apache.org/r/57998/diff/4/ > > > Testing > ------- > > make check (OS X, Fedora25) > > > Thanks, > > Benjamin Bannier > >
