mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.
Date Fri, 08 Jul 2016 16:30:05 GMT

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



Code LGTM. :) Comments mainly on tests.


include/mesos/resources.hpp (line 72)
<https://reviews.apache.org/r/45959/#comment206606>

    For implicit converting constructors it's the convention to add an "annotation".
    
    ```
    /*implicit*/ Resource_(const Resource& _resource)
    ```



include/mesos/resources.hpp (line 99)
<https://reviews.apache.org/r/45959/#comment206595>

    s/adjusted/adjusted or compared/ ?



src/common/resources.cpp (line 856)
<https://reviews.apache.org/r/45959/#comment206596>

    Align the two like this:
    
    ```
    return sharedCount.get() >= that.sharedCount.get() &&
           resource == that.resource;
    ```



src/common/resources.cpp (lines 894 - 898)
<https://reviews.apache.org/r/45959/#comment206597>

    I commented on this previously:
    
    If the substraction results in empty or invalid `Resource_`, it's `Resources::operator-=`'s
responsibility to clean it up.
    
    In other words, It's perfectly fine to have "cpus:100<0>" or "cpus:100<-1>"
by themselves the same way it's fine to have "cpus:-1" by itself. It's `Resources` that doesn't
allow them in it.
    
    Therefore we can remove this from here.



src/common/resources.cpp (line 909)
<https://reviews.apache.org/r/45959/#comment206601>

    Not *contained* here. How about something like *cannot be equal*?



src/common/resources.cpp (line 914)
<https://reviews.apache.org/r/45959/#comment206602>

    s/a shared resource/shared resources/ 
    s/of these resources// (because of redundancy)
    
    ?



src/tests/mesos.hpp (lines 594 - 597)
<https://reviews.apache.org/r/45959/#comment206613>

    Put this in the if block above?



src/tests/resources_tests.cpp (line 2457)
<https://reviews.apache.org/r/45959/#comment206688>

    A general comment is that I see this test (and the one above) mirrors `ScalarSubtraction`
(and `ScalarAddition`) but the code duplication is unfortunate. Some duplication is justified
beacuse we are verifying nonshared resource arithmetic operations still work in `Resources`
when shared resources are present. However let's try to minimize the duplication and length
the these tests.



src/tests/resources_tests.cpp (lines 2459 - 2493)
<https://reviews.apache.org/r/45959/#comment206692>

    The test tends to get pretty long due to these constructions. Utimately it looks like
we are just constructing two `Resources` `r1` and `r2` and testing their `diff`. How about
simply:
    
    ```
    Resource disk = createDiskResource("8192", "role1", "1", "path", None(), true);
    
    Resource r1 = Resources::parse("cpus:40;mem:4096").get() + disk + disk;
    Resource r2 = Resources::parse("cpus:5;mem:512").get() + disk;
    
    Resources diff1 = r1 - r2;
    ```
    
    Note that we are focusing on subtraction here.



src/tests/resources_tests.cpp (lines 2468 - 2474)
<https://reviews.apache.org/r/45959/#comment206689>

    Why add them up since we are testing substraction? Can we construct r1 directly?



src/tests/resources_tests.cpp (lines 2502 - 2507)
<https://reviews.apache.org/r/45959/#comment206651>

    Let's not reuse `diff1` (it's compound assignment and not a `diff` anymore). If we want
to verify that `-=` is equivalent to `-` we can:
    
    ```
    Resources r = r1;
    r1 -= r2;
    
    // The same expectations as those of diff1.
    ```



src/tests/resources_tests.cpp (lines 2509 - 2524)
<https://reviews.apache.org/r/45959/#comment206694>

    So what do these line do? Looks like we are interested in testing the situation where
a shared resource is fully removed from `Resources` after substraction. So let's add a comment
to state the goal and test it explicitly. Also after verifying basic `-` and `-=` already
we can keep the additional expecatations concise.
    
    How about:
    
    ```
    // Verify that the shared disk is removed from Resources after all copies are substracted.
    // r1 has two copies of 'disk' and r2 has one.
    EXPECT_EQ(0, (r1 - r2 - r2).count(disk));
    EXPECT_EQ(0, (r2 - r1).count(disk));
    ```



src/tests/resources_tests.cpp (line 2528)
<https://reviews.apache.org/r/45959/#comment206638>

    The test is a bit long and difficult to follow and it seems to cover a lot of things already
covered in the two tests above.
    
    About `MultipleConsumers`: the tests above already use Resources objects with multiple
copies of the same shared resources so we don't need redo it here.
    
    I suggest for this test we focus on more complex expressions such as `EXPECT_EQ(r1 + r1
- r1, r1)` and additional methods such as `contains()`.
    
    In order to shorten and simplify the test:
    
    - Inline the variable constructions wherever possible.
    - Don't reuse and modify variables like `sum` and `diff` as the test progresses as it's
hard to follow their changed meanings.
    - Don't use `get<Value::Scalar>` to verify each single resource in a Resources object
as we have already done it in the above two tests. Use exepctations to directly compare Resources
objects.



src/tests/resources_tests.cpp (lines 2559 - 2561)
<https://reviews.apache.org/r/45959/#comment206633>

    Space around operators like this:
    
    ```
    EXPECT_EQ(r1 + r1 - r1, r1);
    ```
    
    How about adding one more:
    
    ```
    EXPECT_EQ(r2 + r1 - r1, r2);
    ```



src/tests/resources_tests.cpp (line 2563)
<https://reviews.apache.org/r/45959/#comment206639>

    I suggest we don't reuse variables, perhaps even use `const`.
    
    ```
    const Resources sum = r1 + r2;
    const Resources diff = r1 - r2;
    ```
    
    In the following lines it becomes unclear what each variable means at a given moment anymore...



src/tests/resources_tests.cpp (line 2591)
<https://reviews.apache.org/r/45959/#comment206640>

    To test the "contain relationship with two resoures differ by only the count of one shared
resource" I would just do:
    
    ```
    EXPECT_TRUE(r1.contains(r1 - disk1));
    EXPECT_FALSE((r1 - disk1).contains(r1));
    ```
    
    Also add a comment.



src/tests/resources_tests.cpp (line 2599)
<https://reviews.apache.org/r/45959/#comment206751>

    How about subtractions and contains? Seems like ScalarNonEqualSharedResources and ScalarSharedNonSharedOperations
should be pretty much the same with the only difference being `isShared = true` in one `createDiskResource`
call?



src/tests/resources_tests.cpp (line 2624)
<https://reviews.apache.org/r/45959/#comment206697>

    The test name is not very descriptive so we should add comments to describe the test.\
    
    Call it `ScalarSharedAndNonSharedResources` (following the naming pattern of the previous
test)?



src/tests/resources_tests.cpp (lines 2626 - 2649)
<https://reviews.apache.org/r/45959/#comment206698>

    We can shorten this to:
    
    ```
    Resources r1 = Resources::parse("cpus:1;mem:5).get() + disk1;
    Resources r2 = Resources::parse("cpus:1;mem:5).get() + disk2;
    
    EXPECT_FALSE(r2.contains(r1));
    EXPECT_FALSE(r1.contains(r2));
    ```



src/tests/resources_tests.cpp (line 2651)
<https://reviews.apache.org/r/45959/#comment206753>

    s/sum/r/ when in the compound assignment case.



src/tests/resources_tests.cpp (lines 2661 - 2664)
<https://reviews.apache.org/r/45959/#comment206752>

    s/diff/r/ when using the compound assignment form.
    
    Or,
    
    ```
    EXPECT_EQ((Resources(disk2) - disk1, disk2);
    EXPECT_EQ(Resources(disk1) - disk2, disk1);
    ```


- Jiang Yan Xu


On June 13, 2016, 12:18 a.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> -----------------------------------------------------------
> 
> (Updated June 13, 2016, 12:18 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joris Van Remoortere, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4892
>     https://issues.apache.org/jira/browse/MESOS-4892
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> A new class Resource_ is added that allows 'Resources' to group
> identical shared resource objects together into a single 'Resource_'
> object and tracked by its shared count. Non-shared resource objects
> are not grouped.
> 
> For resource addition and subtraction, the shared count is adjusted for
> shared resources as follows:
> a) Addition: If shared resource is absent from original, then the
>    resource is added initialized with a consumer count of 1. Otherwise,
>    the share count for the shared resource is incremented.
> b) Subtraction: If shared resource's share count is already 1, then
>    the shared resource is removed from the original. Otherwise, its
>    consumer count is decremented.
> 
> Note that v1 changes for shared resources are in the next commit.
> 
> 
> Diffs
> -----
> 
>   include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/master/validation.cpp 7b9c2281b2ab1295211f0dd385cd77947fbd63be 
>   src/tests/mesos.hpp e9361a65eb31cced99e9ed32fd18a65d1bda26e3 
>   src/tests/resources_tests.cpp dc12bd8f1e2da6972bc8aed598811c55d664036e 
> 
> Diff: https://reviews.apache.org/r/45959/diff/
> 
> 
> Testing
> -------
> 
> New tests added to demonstrate arithmetic operations for shared resources with consumer
counts.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>


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