This is an automatically generated e-mail. To reply, visit:

Haven't looked at tests but given the anticipated change in the new revision 
let's do that with the new revision. :)

include/mesos/resources.hpp (line 189)

    According to our discussion with other reviewers the scope the `Resource_` 
is changed from "a step towards a generic C++ resource abstraction" to "an 
internal abstraction to facilitiate managing shared resources".
    We can comment here:
    // An internal abstraction to facilitiate managing shared resources.
    // It allows 'Resources' to group identical shared resource objects 
    // together into a single 'Resource_' object and tracked by its internal
    // counters. Nonshared resource objects are not grouped.

include/mesos/resources.hpp (line 190)

    This is supposed to be hidden in private right?

include/mesos/resources.hpp (line 195)

    No space before `(None())`.

include/mesos/resources.hpp (lines 197 - 200)

    Chatted offline already and this should be 1 for shared resources and None 
for nonshared resources.

include/mesos/resources.hpp (line 203)

    This comment states the obvious fact.
    To add more information we could say:
    // By implicitly converting to Resource we are able to keep Resource_ logic 
    // internal and expose only the protobuf object.

include/mesos/resources.hpp (line 211)

    This indentation is a bit unusual but this fits in one line if we just do:
    bool isShared() const { return shareCount.isSome(); }
    `resource.has_share()` is the same as `shareCount.isSome()`, we should just 
use one.

include/mesos/resources.hpp (lines 225 - 238)

    In the latest design we shouldn't need this anymore.

include/mesos/resources.hpp (lines 240 - 253)

    These can be public if the class itself is private.

include/mesos/resources.hpp (lines 255 - 260)

    As disucssed we should remove this.
    It's hard to explain what 'initialized state' is in this context.

include/mesos/resources.hpp (line 267)

    Could you s/sharedCount/sharedCount/ so it's consistent with the use of the 
word `shared` in protobuf, code and comments elsewhere.

include/mesos/resources.hpp (lines 281 - 282)

    Do we need this?

include/mesos/resources.hpp (lines 303 - 304)

    We talked about its use in tests: I think exposing `size_t 
Resources::count(const Resource&)` is more direct in returning the information 
about how many copies of the shared resources are in the container.
    As for the concerns about leaking the value of the internal counter: I 
think this is fine because the correctness of the Resources arithmetic doesn't 
rely on the callers carefully manipulating the counters correctly or calling 
particular methods based on the result of `count()`. The `count()` method 
simply exposes information for uses other than Resources arithmetic operations 
themselves. Similar to 

include/mesos/resources.hpp (lines 306 - 309)

    As discussed, we'll no longer need this.

include/mesos/resources.hpp (lines 406 - 409)

    As discussed, we'll no longer need this.

include/mesos/v1/resources.hpp (lines 303 - 304)


include/mesos/v1/resources.hpp (lines 453 - 454)

    When do people need to using the vector explicitly?

include/mesos/v1/resources.hpp (line 458)

    s/generated runtime/generated at runtime/.

include/mesos/v1/resources.hpp (lines 484 - 491)

    Should they both exist or we can just replace 
    bool Resources::_contains(const Resource& that) const
    bool Resources::_contains(const Resource_& that) const

src/common/resources.cpp (line 210)

    Remember to update these comments s/ShareInfo/s/SharedInfo/.

src/common/resources.cpp (lines 349 - 350)

    Two identical shared resources with DiskInfo can be substracted right?
    I guess we don't need to specifically mention nonshared resources here.

src/common/resources.cpp (lines 825 - 833)

    Given that We create `Resource_` and assign `sharedCount` internally. These 
errors are never supposed to happen.
    The invalid case here IMO is `shareCount.get() < 0`. This is the equivalent 
        if (resource.scalar().value() < 0) {
          return Error("Invalid scalar resource: value < 0");
    in nonshared resources.

src/common/resources.cpp (lines 841 - 847)

    if (isShared() && sharedCount.get() == 0) {
      return true;
    return Resources::isEmpty(resource);
    because a 0MB shared disk volume is still empty.

src/common/resources.cpp (line 853)

    `isShared() != that.isShared()` is shorter.

src/common/resources.cpp (lines 857 - 858)

    The comment doesn't seem to be describing the condition below, how about
    A shared resource can never contain the other resource with a larger 

src/common/resources.cpp (line 859)

    Since we expose the method `isShared()` it would be clearer if we 
consistently use this method because by calling `shareCount.isSome()` we want 
to see if the resource is shared and `isShared()` literally says it.

src/common/resources.cpp (lines 869 - 879)

    This could be
    return sharedCount == that.sharedCount() && resource == that.resource;

src/common/resources.cpp (line 893)

    Seems like I suggested this on a previous revision but to make it clearer 
what this if condition means, instead of `resource == that.resource` which 
potentially requires a comment 
    // Shared resources are only addable when they have the same
     protobuf value.
    , we could just use `internal::addable(resource, that.resource)`?
    `internal::addable(resource, that.resource)` has a equality check inside 
and we want to have the same logic and comment in a single place.

src/common/resources.cpp (lines 894 - 895)

    We don't need the `shareCount.get() >= 0` and `that.shareCount.get() >= 0` 
checks as the below doesn't need these assumptions to hold.
    `CHECK_SOME(that.shareCount)` is needed because you call 
`that.shareCount.get()` subsequently.

src/common/resources.cpp (lines 897 - 903)

    With shared resources starting with sharedCount == 1 this should always be:
    sharedCount = sharedCount.get() + that.sharedCount.get();

src/common/resources.cpp (line 910)

    As the reverse of `+=` above this method can now have a similar structure.

src/common/resources.cpp (lines 1619 - 1644)

    This method definition basically duplicated the one below.
    Can we implement `Resources& Resources::operator+=(const Resource_& that)` 
in the same way as `Resources& Resources::operator+=(const Resource& that)` 
below and have `Resources& Resources::operator+=(const Resource& that)` simply 
call `*this += Resource_(that);`?
    Can we do the same for other `Resources` operators that take `Resources_`?

src/common/resources.cpp (lines 1651 - 1655)

    resource_ += Resource_(that);
    works in both cases so we don't need to distinguish them right?

src/common/resources.cpp (line 1705)

    Ditto for `Resources& Resources::operator-=(const Resource_& that)` w.r.t. 
not duplicating code.

src/common/resources.cpp (line 1738)

    When we use a variable to catch the `size()` method of a STL container, 
it's conventional to use `size_t`.

src/common/resources.cpp (lines 1742 - 1746)

    No special handling needed.

src/common/resources.cpp (line 1756)

    I would assume the implicitly defined assignment operator does the right 
thing by checking self-assignment so we don't need this here?

src/common/resources.cpp (line 1757)

    `*resources.back()` as `resources.back()` returns an iterator.


    Keep the empty line.

src/common/resources.cpp (lines 1889 - 1890)

    You just need 
    stream << resource_.resource;

src/common/resources.cpp (lines 1891 - 1892)

    `resource_.isShared()` would take care of it.

src/common/resources.cpp (lines 1906 - 1908)

    Wouldn't the version on the left do the right thing?

src/tests/mesos.hpp (line 592)

    A blank line before `return`.

- Jiang Yan Xu

On April 28, 2016, 5:15 p.m., Anindya Sinha wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> -----------------------------------------------------------
> (Updated April 28, 2016, 5:15 p.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 Resoure_ is added to keep track of Resource and its
> consumer count. As a result, Resources maintains a single container
> for all resources.
> 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/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/master/validation.cpp f458100d22ec1f9f10921c1c91b6931a5671e28f 
>   src/tests/mesos.hpp 0f6f541c5d2007a69ad5bd6e884235cd3c0c1be2 
>   src/tests/resources_tests.cpp dc12bd8f1e2da6972bc8aed598811c55d664036e 
>   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