> On May 16, 2016, 6:01 p.m., Jiang Yan Xu wrote: > > include/mesos/resources.hpp, line 190 > > <https://reviews.apache.org/r/45959/diff/3/?file=1365128#file1365128line190> > > > > This is supposed to be hidden in private right?
Done as a part of the refactor. Few of the comments below have been addressed by this change. > On May 16, 2016, 6:01 p.m., Jiang Yan Xu wrote: > > include/mesos/resources.hpp, lines 303-304 > > <https://reviews.apache.org/r/45959/diff/3/?file=1365128#file1365128line303> > > > > 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). Added `Resources::count()` as discussed. I think the analogy is to a `multiset` or `unordered_multiset` (not a `set`). There is a small difference in that the multiple entries (with same keys) in `multiset` or `unordered_multiset` are stored as separate objects, and `count()` accumulates them and returns the count based on the specified key. `Resources::count()` is different since the internal storage (which I agree is not exposed to callers) ensures that a Resource is stored once only and the `count()` returns the sharedCount (if present); or 1 (if not a shared resource); or 0 (otherwise). > On May 16, 2016, 6:01 p.m., Jiang Yan Xu wrote: > > include/mesos/resources.hpp, lines 406-409 > > <https://reviews.apache.org/r/45959/diff/3/?file=1365128#file1365128line406> > > > > As discussed, we'll no longer need this. Updated as discussed. > On May 16, 2016, 6:01 p.m., Jiang Yan Xu wrote: > > include/mesos/v1/resources.hpp, lines 489-496 > > <https://reviews.apache.org/r/45959/diff/3/?file=1365129#file1365129line489> > > > > Should they both exist or we can just replace > > > > ``` > > bool Resources::_contains(const Resource& that) const > > ``` > > > > with > > > > ``` > > bool Resources::_contains(const Resource_& that) const > > ``` > > > > ? Merged the 2 functions, and the following is the only available member function now available: `bool _contains(const Resource_& that) const`. If we have a Resource object, we do a `_contains(Resource_(resource))`. > On May 16, 2016, 6:01 p.m., Jiang Yan Xu wrote: > > src/common/resources.cpp, lines 841-847 > > <https://reviews.apache.org/r/45959/diff/3/?file=1365130#file1365130line841> > > > > ``` > > if (isShared() && sharedCount.get() == 0) { > > return true; > > } > > > > return Resources::isEmpty(resource); > > ``` > > > > because a 0MB shared disk volume is still empty. Good point. > On May 16, 2016, 6:01 p.m., Jiang Yan Xu wrote: > > src/common/resources.cpp, lines 869-879 > > <https://reviews.apache.org/r/45959/diff/3/?file=1365130#file1365130line869> > > > > This could be > > > > ``` > > return sharedCount == that.sharedCount() && resource == that.resource; > > ``` > > > > right? I would leave the existing code instead of shortening it. I think it makes it clearer. > On May 16, 2016, 6:01 p.m., Jiang Yan Xu wrote: > > src/common/resources.cpp, line 1757 > > <https://reviews.apache.org/r/45959/diff/3/?file=1365130#file1365130line1757> > > > > `*resources.back()` as `resources.back()` returns an iterator. std::vector::back() returns a reference or const_reference (not iterator or const_iterator). http://en.cppreference.com/w/cpp/container/vector/back > On May 16, 2016, 6:01 p.m., Jiang Yan Xu wrote: > > src/common/resources.cpp, lines 1890-1891 > > <https://reviews.apache.org/r/45959/diff/3/?file=1365130#file1365130line1890> > > > > You just need > > > > ``` > > stream << resource_.resource; > > ``` > > > > right? Either way is fine. Anyways, I changed it... > On May 16, 2016, 6:01 p.m., Jiang Yan Xu wrote: > > src/common/resources.cpp, lines 1907-1909 > > <https://reviews.apache.org/r/45959/diff/3/?file=1365130#file1365130line1907> > > > > Wouldn't the version on the left do the right thing? Either way is fine. Anyways, I changed it... > On May 16, 2016, 6:01 p.m., Jiang Yan Xu wrote: > > include/mesos/v1/resources.hpp, lines 458-459 > > <https://reviews.apache.org/r/45959/diff/3/?file=1365129#file1365129line458> > > > > When do people need to using the vector explicitly? Removed this. > On May 16, 2016, 6:01 p.m., Jiang Yan Xu wrote: > > src/common/resources.cpp, line 1756 > > <https://reviews.apache.org/r/45959/diff/3/?file=1365130#file1365130line1756> > > > > I would assume the implicitly defined assignment operator does the > > right thing by checking self-assignment so we don't need this here? Most likely. Removed the check since the cost of the = operation is O(1). > On May 16, 2016, 6:01 p.m., Jiang Yan Xu wrote: > > include/mesos/resources.hpp, lines 281-282 > > <https://reviews.apache.org/r/45959/diff/3/?file=1365128#file1365128line281> > > > > Do we need this? Not needed anymore, and hence removed. - Anindya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45959/#review131398 ----------------------------------------------------------- On May 22, 2016, 7:08 a.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45959/ > ----------------------------------------------------------- > > (Updated May 22, 2016, 7:08 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. > > > Diffs > ----- > > include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 > include/mesos/v1/resources.hpp a5ba8fec4c9c3643646308f75a4b28cefe0b3df3 > src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 > src/master/validation.cpp f490b899758bdac9676a6f6939918efa6ac52781 > src/tests/mesos.hpp 79bf1ff16412ce2a510a9b75ab1ac91c1c182653 > 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 > >
