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 Mon, 18 Apr 2016 06:09:53 GMT

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


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