mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 57998: Added ResourceProviderID to Resource protos.
Date Wed, 05 Apr 2017 13:56:44 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message