mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 57998: Added ResourceProviderID to Resource protos.
Date Tue, 04 Apr 2017 21:16:50 GMT

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


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