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

Reply via email to