----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57998/#review171040 -----------------------------------------------------------
src/tests/resources_tests.cpp Lines 1537 (patched) <https://reviews.apache.org/r/57998/#comment243875> 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. src/tests/resources_tests.cpp Lines 1546 (patched) <https://reviews.apache.org/r/57998/#comment243881> I would also check r1's size here. src/tests/resources_tests.cpp Lines 1547 (patched) <https://reviews.apache.org/r/57998/#comment243878> I'd move this to the dedicated 'contains' test src/tests/resources_tests.cpp Lines 1548 (patched) <https://reviews.apache.org/r/57998/#comment243879> looks like this check is for shared resources. Can you remove this check from this test? src/tests/resources_tests.cpp Lines 1559-1560 (patched) <https://reviews.apache.org/r/57998/#comment243880> Ditto. `count` sounds very shared resource related. src/tests/resources_tests.cpp Lines 1572 (patched) <https://reviews.apache.org/r/57998/#comment243882> Ditto. I would prefer we include another type of resources with the same provider ID here in this test. src/tests/resources_tests.cpp Lines 1577-1578 (patched) <https://reviews.apache.org/r/57998/#comment243873> This is contains check, let's factor this out into a separate test. src/tests/resources_tests.cpp Lines 1594 (patched) <https://reviews.apache.org/r/57998/#comment243874> 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. - Jie Yu On March 31, 2017, 9:38 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57998/ > ----------------------------------------------------------- > > (Updated March 31, 2017, 9:38 a.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 82d020e05b303a8248a90bc482b76b54b335146c > 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/3/ > > > Testing > ------- > > make check (OS X, Fedora25) > > > Thanks, > > Benjamin Bannier > >
