----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45959/#review131398 -----------------------------------------------------------
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) <https://reviews.apache.org/r/45959/#comment197276> 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) <https://reviews.apache.org/r/45959/#comment195361> This is supposed to be hidden in private right? include/mesos/resources.hpp (line 195) <https://reviews.apache.org/r/45959/#comment195357> No space before `(None())`. include/mesos/resources.hpp (lines 197 - 200) <https://reviews.apache.org/r/45959/#comment197266> Chatted offline already and this should be 1 for shared resources and None for nonshared resources. include/mesos/resources.hpp (line 203) <https://reviews.apache.org/r/45959/#comment197269> 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) <https://reviews.apache.org/r/45959/#comment195364> 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) <https://reviews.apache.org/r/45959/#comment197317> In the latest design we shouldn't need this anymore. include/mesos/resources.hpp (lines 240 - 253) <https://reviews.apache.org/r/45959/#comment197318> These can be public if the class itself is private. include/mesos/resources.hpp (lines 255 - 260) <https://reviews.apache.org/r/45959/#comment195355> As disucssed we should remove this. It's hard to explain what 'initialized state' is in this context. include/mesos/resources.hpp (line 267) <https://reviews.apache.org/r/45959/#comment195353> 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) <https://reviews.apache.org/r/45959/#comment197522> Do we need this? include/mesos/resources.hpp (lines 303 - 304) <https://reviews.apache.org/r/45959/#comment197336> 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 [set::count()](http://en.cppreference.com/w/cpp/container/unordered_multiset/count). include/mesos/resources.hpp (lines 306 - 309) <https://reviews.apache.org/r/45959/#comment197337> As discussed, we'll no longer need this. include/mesos/resources.hpp (lines 406 - 409) <https://reviews.apache.org/r/45959/#comment197338> As discussed, we'll no longer need this. include/mesos/v1/resources.hpp (lines 303 - 304) <https://reviews.apache.org/r/45959/#comment195418> include/mesos/v1/resources.hpp (lines 453 - 454) <https://reviews.apache.org/r/45959/#comment197487> When do people need to using the vector explicitly? include/mesos/v1/resources.hpp (line 458) <https://reviews.apache.org/r/45959/#comment197488> s/generated runtime/generated at runtime/. include/mesos/v1/resources.hpp (lines 484 - 491) <https://reviews.apache.org/r/45959/#comment197493> Should they both exist or we can just replace ``` bool Resources::_contains(const Resource& that) const ``` with ``` bool Resources::_contains(const Resource_& that) const ``` ? src/common/resources.cpp (line 210) <https://reviews.apache.org/r/45959/#comment197534> Remember to update these comments s/ShareInfo/s/SharedInfo/. src/common/resources.cpp (lines 349 - 350) <https://reviews.apache.org/r/45959/#comment197538> 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) <https://reviews.apache.org/r/45959/#comment197572> 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 of ``` if (resource.scalar().value() < 0) { return Error("Invalid scalar resource: value < 0"); } ``` in nonshared resources. src/common/resources.cpp (lines 841 - 847) <https://reviews.apache.org/r/45959/#comment197597> ``` 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) <https://reviews.apache.org/r/45959/#comment197598> `isShared() != that.isShared()` is shorter. src/common/resources.cpp (lines 857 - 858) <https://reviews.apache.org/r/45959/#comment197637> 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 sharedCount. ``` src/common/resources.cpp (line 859) <https://reviews.apache.org/r/45959/#comment197638> 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) <https://reviews.apache.org/r/45959/#comment197599> This could be ``` return sharedCount == that.sharedCount() && resource == that.resource; ``` right? src/common/resources.cpp (line 893) <https://reviews.apache.org/r/45959/#comment197691> 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) <https://reviews.apache.org/r/45959/#comment197688> 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) <https://reviews.apache.org/r/45959/#comment197650> With shared resources starting with sharedCount == 1 this should always be: ``` sharedCount = sharedCount.get() + that.sharedCount.get(); ``` src/common/resources.cpp (line 910) <https://reviews.apache.org/r/45959/#comment197694> As the reverse of `+=` above this method can now have a similar structure. src/common/resources.cpp (lines 1619 - 1644) <https://reviews.apache.org/r/45959/#comment197735> 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) <https://reviews.apache.org/r/45959/#comment197713> ``` resource_ += Resource_(that); ``` works in both cases so we don't need to distinguish them right? src/common/resources.cpp (line 1705) <https://reviews.apache.org/r/45959/#comment197740> Ditto for `Resources& Resources::operator-=(const Resource_& that)` w.r.t. not duplicating code. src/common/resources.cpp (line 1738) <https://reviews.apache.org/r/45959/#comment197743> 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) <https://reviews.apache.org/r/45959/#comment197748> No special handling needed. src/common/resources.cpp (line 1756) <https://reviews.apache.org/r/45959/#comment197759> 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) <https://reviews.apache.org/r/45959/#comment197753> `*resources.back()` as `resources.back()` returns an iterator. src/common/resources.cpp <https://reviews.apache.org/r/45959/#comment197747> Keep the empty line. src/common/resources.cpp (lines 1889 - 1890) <https://reviews.apache.org/r/45959/#comment197765> You just need ``` stream << resource_.resource; ``` right? src/common/resources.cpp (lines 1891 - 1892) <https://reviews.apache.org/r/45959/#comment197764> `resource_.isShared()` would take care of it. src/common/resources.cpp (lines 1906 - 1908) <https://reviews.apache.org/r/45959/#comment197769> Wouldn't the version on the left do the right thing? src/tests/mesos.hpp (line 592) <https://reviews.apache.org/r/45959/#comment197773> 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 > >
