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



Partial review, mainly in resources.hpp|cpp.

Some high level notes here:

## Semantics
Shared resource should work as **similarly** (both in terms of sharing 
semantics and in sharing code paths) as regular resources as possible.

e.g.,

```
A - A == empty
A + A - A == A
```

These should work the same way for both nonshared and shared resources.

I think the following is the easiest to understand and would make  (slightly 
different than your version in that I don't `sharedCount` being 0 should mean a 
regular shared resource):
- If `sharedCount` is none, it means the reource is nonshared.
- If `sharedCount` is 0, it means the resource is **empty**, this is the 
condition under which `Resources` methods would remove it. 
- If `sharedCount` is >= 1, it means the resource is a normal shared resource. 
`sharedCount` should start with 1 for a shared resource.
- If `sharedCount` < 0, it's **invalid** but this state exists to support 
shared resource arithmetics.

## Level of abstraction
At the Resource abstraction level we shouldn't be concerned about 
"consumers/tasks/roles/frameworks" but only that a `Resource_` supports 
multiple identical shared resources to be managed together by ref counting 
(akin to share_ptr). It's up to the user of this abstraction to build on top of 
this to achieve tracking shared resources in 
offers/allocation/tasks/containers, etc.

## Funtionality
`Resources_` should provide a set of methods/operators like `validate`, 
`isEmpty`, `+=`, etc. 

## Performance
I think overall the approach to store `Resources_` in a list is good. In terms 
of complexity:

Old way:
1) Create from protobuf: O(n) // copy into internal protobuf field.
2) Export to protobuf: O(1) // const ref to internal protobuf field.
3) Usage the protobuf: O(n) // copy.


New way:
1) Create from protobuf: O(n) // copy into interval std::list.
2) Export to protobuf: O(n) // copy from internal std::list.
3) Usage the protobuf: likely O(1) // copy elided most likely.

So overall it doesn't look like there's a performance degradation. 

If we go with an alternative approach in which we store the protobuf data in 
the data member `RepeatedPtrField<Resource> Resources::resources` but store 
`Resource*` in `Resource_` we can potentially achieve the same guaranteed 
performance but it'll require us to keep track of the `Resource*`'s index in 
the `RepeatedPtrField<Resource>` and juggle two data structures in most methods 
which makes it less clean. 

We should verify this in unit tests and benchmarks.


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

    Containers in C++ are mainly associated with things like lists and vectors 
that store a collection of objects.
    
    Here the following is fine: 
    
    ```
    Abstraction for managing the protobuf Resource object.
    ```



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

    Let's use a `class` just like Resources. As a `Resource` wrapper eventually 
most methods for single resource handling should be moved in `Resource_`. Even 
though in this patch we should do the minimum amount of that just to make sure 
resource sharing semantics work, a `class` should be a start.



include/mesos/resources.hpp (lines 58 - 61)
<https://reviews.apache.org/r/45959/#comment192168>

    Name data members without the trailing underscore and constructor arguments 
with a leading underscore.
    
    e.g.,
    
    ```
    Resource_(const Resource& _resource)
      : resource(_resource)
    {
    ...
    }
    ```



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

    This check is too brutal. Plus we can just manage counters internally and 
do not take it as an contructor argument. We always initiate the counter to be 
none or 1 based on the `resource`.
    
    The counters shouldn't be updated directly and I don't think it should be 
even be exposed. `Resource_` should define a set of arithmetic operators to 
update the counters.
    
    e.g.,
    ```
    Resource_& Resource_::operator+=(const Resource_& that);
    ```



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

    No need to define this.



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

    Here `num_consumers` is not generic enough at this level of abstraction. 
    
    Think about the case where the scheduler is tracking the same shared 
resource from multiple offers, it gets combined and it has nothing to do with 
`consumers`, it just has to do with the fact that mutliple copies of it are 
combined.
    
    I think here `sharedCount` is clear enough and generic enough. 
    
    (`shared_ptr` in libstdc++ call its internal counter `shared_count` and we 
use camelCasing, hence `sharedCount`)
    
    Also add a comment about the choice to use `int`. a counter is obviously 
nonnegative but `int` is used to support arithmetic operations. e.g., 
`disk:1<2> - disk:2<5>` (See my comment about `<>`)



include/mesos/resources.hpp (lines 230 - 231)
<https://reviews.apache.org/r/45959/#comment192518>

    Is this used?
    
    Perhaps it's indeed used in the code and I just overlooked but it seems to 
me if `Resources` are used to manage a collection of resources, we shouldn't 
need to separately maintain a list of `Resource_`s elsewhere and later pass it 
to this constructor to get a `Resources`.



include/mesos/resources.hpp (lines 348 - 353)
<https://reviews.apache.org/r/45959/#comment192519>

    We don't need them if we stick to the "let `Resource_` take care of its own 
arithmetic" approach.



include/mesos/resources.hpp (lines 380 - 392)
<https://reviews.apache.org/r/45959/#comment192516>

    Could you verify if these operators were called? It doesn't look like they 
were based on some simple tests I did.
    
    Because of `Resource_::operator const Resource&() const`, you can already 
cast `Resource_` to Resource const ref with no copying. Therefore both of the 
following involve no copying.
    
    ```
    foreach (const Resource& r, resources) {}
    foreach (const Resource_& r, resources) {}
    ```
    
    Could you verify?



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

    The original ordering of the two pairs of methods is fine. We don't need to 
move them.



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

    The implict conversion is already an accepted usage in the codebase so we 
shouldn't change it now. As long as we have test cases to make sure no 
additional copying is happening at unwanted places then we don't need to worry 
about unintended conversions.
    
    Also it's helpful to add a note here because this is a behavior change and 
people may wonder why this is changed from a const ref.
    
    ```
    // Note that the resulted RepeatedPtrField is a const copy generated 
on-the-fly.
    ```



include/mesos/resources.hpp (lines 448 - 464)
<https://reviews.apache.org/r/45959/#comment192681>

    We don't need these.



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

    If we don't need ordering (I don't think we do) then std::list is cheaper 
because of vector's memory allocation.



src/common/resources.cpp (lines 211 - 214)
<https://reviews.apache.org/r/45959/#comment192524>

    Since the counter is not in `Resource`, I don't think this comment is 
accurate. If two `Resource`s have the same sharedness and everything else is 
equal then they are equal. It's probably OK not to add additional comments here 
as I found this pretty self-explanatory.



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

    s/Two Resources/Two non-shared Resources/



src/common/resources.cpp (lines 282 - 283)
<https://reviews.apache.org/r/45959/#comment192527>

    The counter is a Resource_ concept and not a Resource one so I think here 
we should avoid mentioning it.



src/common/resources.cpp (lines 294 - 305)
<https://reviews.apache.org/r/45959/#comment192684>

    This is basically Resource object equality check.
    
    Why don't we do this: At the top of the method, if the left is a shared 
resource, return `left == right`.
    
    Otherwise if right is shared, return false.
    
    Then you can just assume nonshared resources afterwards.



src/common/resources.cpp (lines 367 - 378)
<https://reviews.apache.org/r/45959/#comment192613>

    Ditto for subtractable.



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

    Just to follow a pattern and make it super explicit, add 
    
    ```
    // NOTE: Invalid and zero Resource objects will be ignored.
    ```
    
    just like other constructors.



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

    Ditto.



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

    We should be using `Resource_` here so we can cover both shared resource 
and nonshared resource.



src/common/resources.cpp (lines 892 - 895)
<https://reviews.apache.org/r/45959/#comment192617>

    Shared resources should be handled just like regular resources.
    
    Say there is a shared resource `sr` in both Resources `a` and `b`. 
    
    If `a` has 4 counts of `sr` and `b` has 3, then `a` contains `b` and `b` 
doesn't contain `a`, right?



src/common/resources.cpp (lines 1259 - 1286)
<https://reviews.apache.org/r/45959/#comment192685>

    No need for these.



src/common/resources.cpp (lines 1259 - 1286)
<https://reviews.apache.org/r/45959/#comment192686>

    No need for this.



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

    Also kill this hunk.



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

    Add a blank line above.



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

    The logic that involves updating the counter should be inside `Resource_` 
and it can rely on the `Resource` operators. 
    
    i.e. something like
    
    ```
    Resource_& Resource_::operator+=(const Resource_& that)
    {
      if (!isShared() && !that.isShared()) {
        this.resource += that.resource;  
      } else if (resource == that.resource) {
        sharedCount += that.sharedCount;
      }
      
      return *this;
    }
    ```
    
    Furthermore, comparing the two methods
    
    ```
    Resources& Resources::operator+=(const Resource& that)
    Resources& Resources::operator+=(const Resource_& that)
    ```
    
    Since Resource to implicitly convertible to Resource_, we don't need the 
first method.



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

    Per the comment above, we can modify this method to take `const Resource_&` 
and keep most of the method's body unchanged.



src/common/resources.cpp (lines 1596 - 1603)
<https://reviews.apache.org/r/45959/#comment192665>

    So there:
    
    ```
    if (internal::addable(resource_.resource, that)) {
      // This applies to both shared and nonshared resources.
      resource_ += that; // This should be implemented in Resource_.
      found = true;
      break;
    }
    ```
    
    Most of these don't have to change.



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

    Here simply:
    
    ```
    resources.push_back(that);
    ```



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

    s/that.resources/that/



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

    Use the same approach as in `+=`.



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

    Expose this to the header and friend it from the `Resource_` class so it 
can acess the private counter.



src/common/resources.cpp (lines 1850 - 1851)
<https://reviews.apache.org/r/45959/#comment192692>

    The two ifs are equivalent. 
    
    `Resource_` should have a `isShared()` method so the `<<` and other callers 
have one canonical way of doing this.
    
    e.g.,
    ```
    bool isShared()
    {
      reutrn sharedCount.isSome();
    }
    ```



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

    `()` is used by roles. Let's use `<>` here. I know that we are running out 
of bracket symbols soon but at least we are OK for now.



src/common/resources.cpp (lines 1853 - 1855)
<https://reviews.apache.org/r/45959/#comment192693>

    Don't add anything special for nonshared resource since it's optional.


- Jiang Yan Xu


On April 11, 2016, 10 p.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> -----------------------------------------------------------
> 
> (Updated April 11, 2016, 10 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4892
>     https://issues.apache.org/jira/browse/MESOS-4892
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> A new struct Resoure_ is added to keep track of Resource and its
> consumer count. As a result, Resources maintains a single container
> for all resources. Private operators for addition and subtraction
> to/from Resource_ have been added.
> 
> All resources have consumer counts that is tracked within Resources. For
> resource addition and subtraction, the consumer counts are adjusted for
> shared resources as follows:
> a) Addition: If shared resource is absent from original, then the
>    resource is added with a consumer count of 0. Otherwise, the consumer
>    count for the shared resource is incremented by 1.
> b) Subtraction: If shared resource's consumer count is already 0, then
>    the shared resource is removed from the original. Otherwise, its
>    consumer count is decremented by 1.
> 
> 
> Diffs
> -----
> 
>   include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
>   include/mesos/v1/resources.hpp a5ba8fec4c9c3643646308f75a4b28cefe0b3df3 
>   src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/examples/dynamic_reservation_framework.cpp 
> 8f00bcf50c25cf46c3dc32e3e77370b39fbd46bc 
>   src/examples/no_executor_framework.cpp 
> f578edfd99f3b7adf19cf06eab20696532c7b67d 
>   src/examples/persistent_volume_framework.cpp 
> b4faa0ee25dc3a72c17ef2b0640a3695423ef79a 
>   src/examples/test_framework.cpp 79113fbe47fda0912f0b01dc10429495a96ba8b8 
>   src/examples/test_http_framework.cpp 
> cba520e326ff8b0b4ed36a0f4cea6879b57f400c 
>   src/hook/manager.cpp 17a42f8362f58f0857acabeb2c3113354589fa1b 
>   src/master/http.cpp 626c88f85910c4e476194f203341c4db7053e0f0 
>   src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
>   src/master/validation.cpp 504cd9b8bd5d40bb591b7aa5a23bd74cc210c2fc 
>   src/slave/containerizer/containerizer.cpp 
> d0cae79834e451594d7675f00c5f7d2d2cd3a264 
>   src/slave/containerizer/external_containerizer.cpp 
> cf4384cce44172a028c890f52f71ceb8ae109383 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
>   src/slave/state.hpp 0de2a4ee4fabaad612c4526166157b001c380bdb 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 7accd32fba5eed196a82b1a171cb16d37b9e0539 
>   src/tests/containerizer/isolator_tests.cpp 
> 7b4d47bd9e99b71269093d7c11559f3b74a3e22b 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 21ad1e1c53316a3bb6d914aa228ccf3658acdfbf 
>   src/tests/containerizer/runtime_isolator_tests.cpp 
> a11a3ffb1df1c5bb760041125c83b7b66d44300b 
>   src/tests/fetcher_cache_tests.cpp 9ffcd2375f1203bd3d7c5d0cc898e955d5cb124e 
>   src/tests/hierarchical_allocator_tests.cpp 
> a5dd57a4e0c244fb099433eb7b5777982698ebfd 
>   src/tests/master_quota_tests.cpp e4a65bf09c8fdd2d6b6161042c3702a8cc4cd454 
>   src/tests/master_tests.cpp a5b21d3d60f944fd52ceacb4bbbad2613f384db7 
>   src/tests/master_validation_tests.cpp 
> 8a5bf9477596f13b2fb3a1348337ad2fe53a034d 
>   src/tests/mesos.hpp 20370a277d55efeea8daae7ea5e2f6575b5a2d62 
>   src/tests/oversubscription_tests.cpp 
> 23671746da2ac505d75bc2bd59114697d9161d52 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 9b8ad34469c0c9a986aa60f3a52584a3a9eabb2b 
>   src/tests/persistent_volume_tests.cpp 
> d246f35046fff469b847c908de2b305ae629212f 
>   src/tests/registrar_tests.cpp f18e8030f69d8ebf8de81ff03632106e08823df1 
>   src/tests/reservation_endpoints_tests.cpp 
> 2e0f6c1aba95d918b8c42219ee97f79f1070d56e 
>   src/tests/resources_tests.cpp dc12bd8f1e2da6972bc8aed598811c55d664036e 
>   src/tests/role_tests.cpp 24959d6e0f83ef7b62b0586be18661aa3cac91dd 
>   src/tests/slave_recovery_tests.cpp 79132344be3bcd2bda54357cd5e7e0c59a766fd8 
>   src/tests/slave_tests.cpp 4a576b98d1cc58072626ac2c41c599bd3c8385c5 
>   src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 
> 
> 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
> 
>

Reply via email to