mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 69601: Added a `Resources` method `contains(ResourceQuantities)`.
Date Thu, 03 Jan 2019 17:14:36 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69601/#review211638
-----------------------------------------------------------




include/mesos/resources.hpp
Lines 45 (patched)
<https://reviews.apache.org/r/69601/#comment297027>

    This is a public header and I don't think the internal headers get installed so this would
break any use of the public headers?
    
    Can you forward declare instead?



src/common/resources.cpp
Lines 1517 (patched)
<https://reviews.apache.org/r/69601/#comment297028>

    Do we need the iterator? Can we use a foreach auto loop here?



src/common/resources.cpp
Lines 1518 (patched)
<https://reviews.apache.org/r/69601/#comment297030>

    s/remainingAmount/remaining/



src/common/resources.cpp
Lines 1523-1525 (patched)
<https://reviews.apache.org/r/69601/#comment297031>

    Maybe we can make this a bit more tidy?
    
    ```
            case Value::SCALAR: remaining -= r.scalar().value();  break;
            case Value::SET:    remaining -= r.set().item_size(); break;
            case ...
    ```



src/common/resources.cpp
Lines 1529 (patched)
<https://reviews.apache.org/r/69601/#comment297029>

    Why the comment here but not below on the same optimization?
    
    It seems fine without the comment in both places to me



src/tests/resources_tests.cpp
Lines 2054 (patched)
<https://reviews.apache.org/r/69601/#comment297032>

    Maybe we can make the tests more readable with some better variable names or lambdas that
allow us to see the entire comparision on one line?
    
    E.g.
    
    ```
    EXPECT_TRUE(Resources().contains(ResourceQuantities());
    
    EXPECT_FALSE(nonEmptyResources.contains(emptyQuantities);
    
    auto resources = [](const string& s) { ... };
    auto quantities = [](const string& s) { ... };
    
    // Can easily read what's being checked:
    EXPECT_TRUE(resources("cpus:1;mem:2")
                  .contains(quantities("cpus:0.1;mem:1"));
    ```


- Benjamin Mahler


On Dec. 31, 2018, 5:52 a.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69601/
> -----------------------------------------------------------
> 
> (Updated Dec. 31, 2018, 5:52 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This method checks if the quantities of this `Resources` is a
> superset of the given `ResourceQuantities`. If a `Resource` object
> is `SCALAR` type, its quantity is its scalar value. For `RANGES`
> and `SET` type, their quantities are the number of different
> instances in the range or set. For example, "range:[1-5]" has a
> quantity of 5 and "set:{a,b}" has a quantity of 2.
> 
> Also added a dedicated test.
> 
> 
> Diffs
> -----
> 
>   include/mesos/resources.hpp 36ccf0e1135ce15898d31db51c80fa7b5826907b 
>   include/mesos/v1/resources.hpp 1a9ea44e78d985286e04e040879d022838ddcc3f 
>   src/common/resources.cpp 758b5a2fac101bfb0a07ba76c204b02d1554b3ac 
>   src/tests/resources_tests.cpp 5337a5c9af9431cc1c7822c78945ffc369488900 
>   src/v1/resources.cpp d717f5327a45cfe9bc075bc3acf37316223d389d 
> 
> 
> Diff: https://reviews.apache.org/r/69601/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


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