Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Jiang Yan Xu]

> On July 28, 2016, 6:14 p.m., Jiang Yan Xu wrote:
> > src/tests/resources_tests.cpp, line 2727
> > 
> >
> > I apologize for my typo in the previous review, I was going to suggest 
> > a `count` test because it's not covered by existing tests for non-shared 
> > resources and there are some interesting scenarios here.
> > 
> > I was thinking of something like this.
> > 
> > ```
> > TEST(ResourcesTest, Count)
> > {
> >   Resource sharedDisk = createDiskResource(
> >   "100", "role1", "2", "path2", None(), true);
> >   EXPECT_EQ(1, (Resources(sharedDisk)).count(shared));
> >   EXPECT_EQ(2, (Resources(sharedDisk) + shared).count(shared));
> >   
> >   // The summation is invalid and a no-op for non-shared disks so the 
> >   // count remains 1.
> >   Resource nonsharedDisk = createDiskResource("100", "role1", "2", 
> > "path2");
> >   EXPECT_EQ(1, (Resources(nonsharedDisk)).count(nonsharedDisk));
> >   EXPECT_EQ(1, (Resources(nonsharedDisk) + 
> > nonsharedDisk).count(nonsharedDisk));
> >   
> >   // After the summation the scalar changes so the count is 0.
> >   Resources cpus = Resources::parse("cpus:1").get();
> >   EXPECT_EQ(1, (cpus).count(cpus));
> >   EXPECT_EQ(0, (cpus + cpus).count(cpus));
> > }
> > ```
> > 
> > A dedicated `Contains` test would have been nice if it can simpify 
> > other tests and if there was one it should probably be no longer or more 
> > complex than the `Count` test. However given the way other tests are 
> > written perhaps don't stress about it.

I was suggesting killing the `Contains` test (in addition to adding the 
`Count`) because it's a bit complicated IMO and the things it tested are 
covered in other tests. Will not include `SharedResourcesTest.Contains` for 
now, we can chat about adding it back if necessary in a later patch.


- Jiang Yan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/#review143997
---


On July 29, 2016, 11:59 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated July 29, 2016, 11:59 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 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.
> 
> Note that v1 changes for shared resources are in the next commit.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 6638c8f79e45c0ff3efa4342e30478d2e1e4740b 
>   src/common/resources.cpp 468581da550bcabf44fbaba8897d5fbbc330c2cb 
>   src/master/validation.cpp 52002beac29c371411348cb026a216e99ac96ab2 
>   src/tests/mesos.hpp 51c66f175c80ebacd5af230222ea7e4c81dfc1e8 
>   src/tests/resources_tests.cpp 4111e080b84079e100b731c9a56861b204f17388 
> 
> 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
> 
>

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/#review144059
---


Fix it, then Ship it!




I'll commit with the following minor adjustments.


include/mesos/resources.hpp (lines 460 - 461)






include/mesos/resources.hpp (lines 481 - 484)


Moved them to below the comments below as it applies to them as well. I 
don't think we actually need 

```
void add(const Resource& r);
void subtract(const Resource& r);
```

to be public to let's address them separately.



src/tests/resources_tests.cpp (line 2564)


Moved the tests for shared resources to above the benchmarks so the unit 
tests and the benchmarks are clearly delimited.



src/tests/resources_tests.cpp (line 2566)


Added a comment here and in similar places to explain the `true` argument.

```
// Shared persistent volume.
```



src/tests/resources_tests.cpp (line 2590)


s/same/the same/



src/tests/resources_tests.cpp (line 2616)


s/same/the same/



src/tests/resources_tests.cpp (line 2685)


s/differs/differ/



src/tests/resources_tests.cpp (lines 2689 - 2691)


Renamed disk1 and disk2 as `sharedDisk` and `nonSharedDisk` so their 
sharedness is more self-explanatory.



src/tests/resources_tests.cpp (line 2753)


Pulled this test to the top of the shared resources tests as this is a 
basic test that establishes the validity of basic operations on shared 
resources.



src/tests/resources_tests.cpp (line 2755)


Added

```
  // The summation of identical shared resources is valid and
  // the result is reflected in the count.
```


- Jiang Yan Xu


On July 29, 2016, 11:59 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated July 29, 2016, 11:59 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 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.
> 
> Note that v1 changes for shared resources are in the next commit.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 6638c8f79e45c0ff3efa4342e30478d2e1e4740b 
>   src/common/resources.cpp 468581da550bcabf44fbaba8897d5fbbc330c2cb 
>   src/master/validation.cpp 52002beac29c371411348cb026a216e99ac96ab2 
>   src/tests/mesos.hpp 51c66f175c80ebacd5af230222ea7e4c81dfc1e8 
>   src/tests/resources_tests.cpp 4111e080b84079e100b731c9a56861b204f17388 
> 
> 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
> 
>

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/
---

(Updated July 29, 2016, 7:09 p.m.)


Review request for mesos, Benjamin Mahler, Joris Van Remoortere, and Jiang Yan 
Xu.


Changes
---

Rebased and addressed review comments.


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.

Note that v1 changes for shared resources are in the next commit.


Diffs (updated)
-

  include/mesos/resources.hpp 88a9feabf66ed34e7e5b1c6cb7e831818e7f7883 
  src/common/resources.cpp 3dbff24d6859d3b1ed8589cec50170a5202cfbcb 
  src/master/validation.cpp 52002beac29c371411348cb026a216e99ac96ab2 
  src/tests/mesos.hpp 51c66f175c80ebacd5af230222ea7e4c81dfc1e8 
  src/tests/resources_tests.cpp 4111e080b84079e100b731c9a56861b204f17388 

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

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

> On July 29, 2016, 1:14 a.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, lines 848-850
> > 
> >
> > Just trying to see if the comments can be made more concise:
> > 
> > ```
> > Assuming the wrapped Resource objects are equal, the 'contains' 
> > relationship is determined by the relationship of the counters for shared 
> > resources.
> > ```

It did not make the comments much more concise but I updated the comments 
though.


> On July 29, 2016, 1:14 a.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, line 856
> > 
> >
> > Here an `else` clause is more explicit.
> > 
> > ```
> > else {
> >   return internal::contains(resource, that.resource);
> > }
> > ```

I think both are equivalent.


> On July 29, 2016, 1:14 a.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, lines 255-279
> > 
> >
> > Why the change from the last revision?
> > 
> > Sharedness check at the top allows us to short-circuit the checking for 
> > shared resources and allow us to not have to consider shared resources as a 
> > possbility in subsequent if conditions which is better IMO both for 
> > simplicity and performance (even if the difference in performance may not 
> > be significant).

Whoops. This was based on a comment from Klaus Ma, but the earlier 
implementation addressed that since `bool operator==(const Resource& left, 
const Resource& right)` has all the checks. I misread the `operator==()` to 
only check for values.
I will revert this to original version.


> On July 29, 2016, 1:14 a.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, lines 323-346
> > 
> >
> > Ditto.

Reverted to earlier version.


- Anindya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/#review143997
---


On July 28, 2016, 10:42 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated July 28, 2016, 10:42 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 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.
> 
> Note that v1 changes for shared resources are in the next commit.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 88a9feabf66ed34e7e5b1c6cb7e831818e7f7883 
>   src/common/resources.cpp 3dbff24d6859d3b1ed8589cec50170a5202cfbcb 
>   src/master/validation.cpp 52002beac29c371411348cb026a216e99ac96ab2 
>   src/tests/mesos.hpp 51c66f175c80ebacd5af230222ea7e4c81dfc1e8 
>   src/tests/resources_tests.cpp 4111e080b84079e100b731c9a56861b204f17388 
> 
> 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
> 
>

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

> On July 20, 2016, 2:36 a.m., Klaus Ma wrote:
> > src/common/resources.cpp, lines 308-316
> > 
> >
> > I think we can check name,type, role firstly, then check SharedInfo.

Although I updated it based on your comment, I think it was not needed since 
`left == right` does those checks. I will revert it in the next update.


- Anindya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/#review142892
---


On July 28, 2016, 10:42 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated July 28, 2016, 10:42 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 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.
> 
> Note that v1 changes for shared resources are in the next commit.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 88a9feabf66ed34e7e5b1c6cb7e831818e7f7883 
>   src/common/resources.cpp 3dbff24d6859d3b1ed8589cec50170a5202cfbcb 
>   src/master/validation.cpp 52002beac29c371411348cb026a216e99ac96ab2 
>   src/tests/mesos.hpp 51c66f175c80ebacd5af230222ea7e4c81dfc1e8 
>   src/tests/resources_tests.cpp 4111e080b84079e100b731c9a56861b204f17388 
> 
> 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
> 
>

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/#review143997
---



This is done before the you rebased but it should be straightforward once we 
address these remaining issues and we should be good to go!


include/mesos/resources.hpp (line 64)


Perhaps a short note:

```
// The rest of the private section is below the public section. We need to 
define Resource_ first because the public typedefs below depend on it.
```



include/mesos/resources.hpp (line 95)


Can we reword it as

```
The `Resource_` arithmetric and comparison operators and `contains()` 
method require...
```



include/mesos/resources.hpp (line 96)


I didn't raise this earlier but to maintain consistency in comments let's 
just do

s/shared property/sharedness/



include/mesos/resources.hpp (line 101)


s/comparison/`contains()` method/

comparisions (i.e., ==, !=) are operators too.



include/mesos/resources.hpp (line 471)


s/is/are/



src/common/resources.cpp (lines 255 - 279)


Why the change from the last revision?

Sharedness check at the top allows us to short-circuit the checking for 
shared resources and allow us to not have to consider shared resources as a 
possbility in subsequent if conditions which is better IMO both for simplicity 
and performance (even if the difference in performance may not be significant).



src/common/resources.cpp (lines 323 - 346)


Ditto.



src/common/resources.cpp (lines 842 - 843)


Just trying to see if we can make the comments more concise.

```
// Same sharedness required.
```



src/common/resources.cpp (lines 848 - 850)


Just trying to see if the comments can be made more concise:

```
Assuming the wrapped Resource objects are equal, the 'contains' 
relationship is determined by the relationship of the counters for shared 
resources.
```



src/common/resources.cpp (line 856)


Here an `else` clause is more explicit.

```
else {
  return internal::contains(resource, that.resource);
}
```



src/common/resources.cpp (lines 900 - 901)


Just trying to see if we can make the comments more concise.

```
// Same sharedness required.
```



src/common/resources.cpp (line 979)


Looks like we don't need `const Resource& resource = resource_;` and we can 
just do:

```
if (resource_.resource == that) {
...
}
```

?



src/tests/resources_tests.cpp (line 2675)


In my previous comment I was suggesting that let's make sections of the 
test not depend on one another. i.e., r2 doesn't depend on r1. (Of course as a 
reader I still need to know the definition of disk1, disk2 and disk3). Speaking 
of which, disk3 and disk2 are the same so perhaps we don't need a disk3 (once 
we established in simple tests that the equality operator works).

If it's written this way, it's then very clear to me what exactly we are 
testing in each section and whether some tests are redundant.

e.g.,

```
Resources r2 = Resources(disk1) + disk2; (Addition)

...

Resources r3 = Resources(disk1) + disk2 - disk1; (Can substract)

...

Resources r4 = Resources(disk1) - disk2; (Cannot substract)
```



src/tests/resources_tests.cpp (line 2700)


Seems like adding disk1 twice doesn't particularly help with anything? For 
symmetry maybe just 

```
Resources r1 = Resources::parse("cpus:1;mem:5").get() + disk1;

Resources r2 = Resources::parse("cpus:1;mem:5").get() + disk2;
```



src/tests/resources_tests.cpp (line 2702)


Kill this line. We don't have it for r2 and we it's already covered in 
other tests.



src/tests/resources_tests.cpp (line 2707)


Simple heading.

// r1 and r2 don't contain each other because of disk1 and disk2's 
different sharedness.



src/tests/resources_tests.cpp (line 2710)

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/
---

(Updated July 28, 2016, 10:42 p.m.)


Review request for mesos, Benjamin Mahler, Joris Van Remoortere, and Jiang Yan 
Xu.


Changes
---

Rebase with master.


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.

Note that v1 changes for shared resources are in the next commit.


Diffs (updated)
-

  include/mesos/resources.hpp 88a9feabf66ed34e7e5b1c6cb7e831818e7f7883 
  src/common/resources.cpp 3dbff24d6859d3b1ed8589cec50170a5202cfbcb 
  src/master/validation.cpp 52002beac29c371411348cb026a216e99ac96ab2 
  src/tests/mesos.hpp 51c66f175c80ebacd5af230222ea7e4c81dfc1e8 
  src/tests/resources_tests.cpp 4111e080b84079e100b731c9a56861b204f17388 

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

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

> On July 20, 2016, 2:36 a.m., Klaus Ma wrote:
> > include/mesos/resources.hpp, line 124
> > 
> >
> > Seems `Option` is not necessary.

sharedCount is None when it is is regular non-shared resource, and is an 
integer for a shared resource which references to the number of copies of the 
shared resource. So, I think `Option` should be fine here.


> On July 20, 2016, 2:36 a.m., Klaus Ma wrote:
> > include/mesos/resources.hpp, line 484
> > 
> >
> > Why not `list`? It may avoid un-necessary resizing.

`std::list` does a memory allocation for every insert and a deallocation on 
every remove. `std::vector` reserves memory for certain number of entries 
(dependent on platform and implementation) but not on every insert (and hence 
memory allocation is done in a batch, and same for memory deallocation).
To alleviate move/copy operations on insert and remove from `std::vector`, we 
always do a `std::vector::push_back()` on insertion which is O(1), and for 
erase we swap the entry with the last entry using `std::vector::back()` and 
remove the last entry with a `std::vector::pop_back()` resulting in O(1).


> On July 20, 2016, 2:36 a.m., Klaus Ma wrote:
> > include/mesos/resources.hpp, line 84
> > 
> >
> > I think this's dangous, the developer need to check this converting and 
> > decide which `operator` is used. If miss used, it's hard to debuging.

Can you please explain why this is dangerous? This operator implicitly converts 
a `Resource_` to return the corresponding `Resource`.
Since `Resource_` is nested private class (and is hence hidden), callers would 
care about `Resource` only (this would ensure backward compatibility with 
existing code).


> On July 20, 2016, 2:36 a.m., Klaus Ma wrote:
> > src/common/resources.cpp, line 882
> > 
> >
> > one `isShared()` is enough. `isShared` is checked in `subtractable`.

Updated in `Resources::Resource_& Resources::Resource_::operator+=(const 
Resource_& that)` as well as in `Resources::Resource_& 
Resources::Resource_::operator-=(const Resource_& that)`.


- Anindya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/#review142892
---


On July 19, 2016, 10:51 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated July 19, 2016, 10:51 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 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.
> 
> Note that v1 changes for shared resources are in the next commit.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
>   src/common/resources.cpp b1bd2784aefdebf91892638b40b215373b998574 
>   src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
>   src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
>   src/tests/resources_tests.cpp 4111e080b84079e100b731c9a56861b204f17388 
> 
> 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
> 
>

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/
---

(Updated July 21, 2016, 12:12 a.m.)


Review request for mesos, Benjamin Mahler, Joris Van Remoortere, and Jiang Yan 
Xu.


Changes
---

updates based on review comments.


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.

Note that v1 changes for shared resources are in the next commit.


Diffs (updated)
-

  include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
  src/common/resources.cpp b1bd2784aefdebf91892638b40b215373b998574 
  src/master/validation.cpp 52002beac29c371411348cb026a216e99ac96ab2 
  src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
  src/tests/resources_tests.cpp 4111e080b84079e100b731c9a56861b204f17388 

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

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Klaus Ma]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/#review142892
---




include/mesos/resources.hpp (line 84)


I think this's dangous, the developer need to check this converting and 
decide which `operator` is used. If miss used, it's hard to debuging.



include/mesos/resources.hpp (line 124)


Seems `Option` is not necessary.



include/mesos/resources.hpp (line 479)


Why not `list`? It may avoid un-necessary resizing.



src/common/resources.cpp (lines 308 - 316)


I think we can check name,type, role firstly, then check SharedInfo.



src/common/resources.cpp (line 882)


one `isShared()` is enough. `isShared` is checked in `subtractable`.


- Klaus Ma


On July 20, 2016, 6:51 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated July 20, 2016, 6:51 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.
> 
> Note that v1 changes for shared resources are in the next commit.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
>   src/common/resources.cpp b1bd2784aefdebf91892638b40b215373b998574 
>   src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
>   src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
>   src/tests/resources_tests.cpp 4111e080b84079e100b731c9a56861b204f17388 
> 
> 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
> 
>

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

> On July 19, 2016, 5:03 p.m., Jiang Yan Xu wrote:
> > It would be helpful to add a test just for contains. i.e., to test how 
> > `contains` works with nonshared resources and how it works with simple 
> > shared resources (could be constructed from createDiskResources with no 
> > arithmetic operations).
> > 
> > Also as a followup add a benchmark for resource operations. (Let's check 
> > with the IBM folks on their progress in adding DiskInfo and Reservation
> 
> Jiang Yan Xu wrote:
> Finishing my sentense above: "Diskinfo and ReservationInfo" to resources 
> benchmarks)"

Added a test `SharedResourcesTest.Contains` to verify `contains()` semantics 
with regular and shared resources.
Regarding benchmark for resources, that has already been added to 
https://reviews.apache.org/r/49571/.


> On July 19, 2016, 5:03 p.m., Jiang Yan Xu wrote:
> > src/tests/resources_tests.cpp, line 2615
> > 
> >
> > How about also throwing in a check for `count`:
> > 
> > ```
> > EXPECT_TRUE(diff.count(disk));
> > ```

I think you mean check for `contains(disk)`. Added.


> On July 19, 2016, 5:03 p.m., Jiang Yan Xu wrote:
> > src/tests/resources_tests.cpp, line 2647
> > 
> >
> > Seems like this should fail?
> > 
> > This should be true?
> > ```
> > EXPECT_EQ(r2 - r1 + r1, r2);
> > ```

```
Resources r1 = Resources::parse("cpus:2;mem:10").get() + disk + disk + disk + 
disk;
Resources r2 = Resources::parse("cpus:2;mem:10").get() + disk + disk + disk;
```

So, `EXPECT_EQ(r2 - r1 + r1, r1)` works as follows:
1) Since the `r2.count(disk) = 3` and `r1.count(disk) = 4`, so `r2 - r1` is an 
empty `Resources` object (since unshared resources are same).
2) So, when we add `r1` to that empty `Resources` object, we should get `r1`.


- Anindya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/#review142652
---


On July 18, 2016, 2:29 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated July 18, 2016, 2:29 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 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.
> 
> Note that v1 changes for shared resources are in the next commit.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
>   src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
>   src/tests/resources_tests.cpp 40d290ac540d26373c5fb7c2a93d27d1aa61d722 
> 
> 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
> 
>

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/
---

(Updated July 19, 2016, 10:51 p.m.)


Review request for mesos, Benjamin Mahler, Joris Van Remoortere, and Jiang Yan 
Xu.


Changes
---

Addressed review comments.


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.

Note that v1 changes for shared resources are in the next commit.


Diffs (updated)
-

  include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
  src/common/resources.cpp b1bd2784aefdebf91892638b40b215373b998574 
  src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
  src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
  src/tests/resources_tests.cpp 4111e080b84079e100b731c9a56861b204f17388 

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

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Jiang Yan Xu]

> On July 19, 2016, 10:03 a.m., Jiang Yan Xu wrote:
> > It would be helpful to add a test just for contains. i.e., to test how 
> > `contains` works with nonshared resources and how it works with simple 
> > shared resources (could be constructed from createDiskResources with no 
> > arithmetic operations).
> > 
> > Also as a followup add a benchmark for resource operations. (Let's check 
> > with the IBM folks on their progress in adding DiskInfo and Reservation

Finishing my sentense above: "Diskinfo and ReservationInfo" to resources 
benchmarks)"


- Jiang Yan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/#review142652
---


On July 18, 2016, 7:29 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated July 18, 2016, 7:29 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.
> 
> Note that v1 changes for shared resources are in the next commit.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
>   src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
>   src/tests/resources_tests.cpp 40d290ac540d26373c5fb7c2a93d27d1aa61d722 
> 
> 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
> 
>

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/#review142652
---



It would be helpful to add a test just for contains. i.e., to test how 
`contains` works with nonshared resources and how it works with simple shared 
resources (could be constructed from createDiskResources with no arithmetic 
operations).

Also as a followup add a benchmark for resource operations. (Let's check with 
the IBM folks on their progress in adding DiskInfo and Reservation


src/tests/resources_tests.cpp (lines 2602 - 2603)


Why adding the two `Resources::parse()` results here?

Can we directly

```
Resources r1 = Resources::parse("cpus:40;mem:4096").get() + disk + disk;
```

? This would fit in one line (if it doesn't, use two space indentation).



src/tests/resources_tests.cpp (line 2607)


s/diff1/diff/ (due to the rename suggestion below).



src/tests/resources_tests.cpp (line 2615)


How about also throwing in a check for `count`:

```
EXPECT_TRUE(diff.count(disk));
```



src/tests/resources_tests.cpp (line 2617)


s/diff2/r/ as it's technically not a diff.



src/tests/resources_tests.cpp (lines 2623 - 2626)


For completeness let's verify the (pre-)condition before we take the diff.

i.e., add the following above this block.

```
EXPECT_EQ(2, r1.count(disk));
EXPECT_TRUE(r1.contains(disk));
EXPECT_EQ(1, r2.count(disk));
EXPECT_TRUE(r2.contains(disk));
```



src/tests/resources_tests.cpp (lines 2634 - 2637)


2 space indentation.



src/tests/resources_tests.cpp (line 2647)


Seems like this should fail?

This should be true?
```
EXPECT_EQ(r2 - r1 + r1, r2);
```



src/tests/resources_tests.cpp (lines 2650 - 2660)


These feel redundant as ScalarSubtractionShared has covered this case. 
Remove them?



src/tests/resources_tests.cpp (line 2670)


s/ScalarSharedNonEqualSharedOperations/ScalarNonEqualSharedOperations/



src/tests/resources_tests.cpp (lines 2679 - 2700)


Can we directly create r1, r2, r3 and r4 from disk1, disk2 and disk3, i.e., 
`Resources r2 = disk1 + disk2;` so each test block is independent so the test 
is easier to follow?


- Jiang Yan Xu


On July 18, 2016, 7:29 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated July 18, 2016, 7:29 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.
> 
> Note that v1 changes for shared resources are in the next commit.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
>   src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
>   src/tests/resources_tests.cpp 40d290ac540d26373c5fb7c2a93d27d1aa61d722 
> 
> 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
> 
>

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/
---

(Updated July 18, 2016, 2:29 p.m.)


Review request for mesos, Benjamin Mahler, Joris Van Remoortere, and Jiang Yan 
Xu.


Changes
---

rebased.


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.

Note that v1 changes for shared resources are in the next commit.


Diffs (updated)
-

  include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
  src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
  src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
  src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
  src/tests/resources_tests.cpp 40d290ac540d26373c5fb7c2a93d27d1aa61d722 

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

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/
---

(Updated July 17, 2016, 8:44 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 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.

Note that v1 changes for shared resources are in the next commit.


Diffs
-

  include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
  src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
  src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
  src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
  src/tests/resources_tests.cpp dc12bd8f1e2da6972bc8aed598811c55d664036e 

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

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/
---

(Updated July 8, 2016, 11:03 p.m.)


Review request for mesos, Benjamin Mahler, Joris Van Remoortere, and Jiang Yan 
Xu.


Changes
---

Updates on review comments including cleanup in some of the tests.


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.

Note that v1 changes for shared resources are in the next commit.


Diffs (updated)
-

  include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
  src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
  src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
  src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
  src/tests/resources_tests.cpp dc12bd8f1e2da6972bc8aed598811c55d664036e 

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

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

> On July 8, 2016, 4:30 p.m., Jiang Yan Xu wrote:
> > src/tests/resources_tests.cpp, line 2563
> > 
> >
> > I suggest we don't reuse variables, perhaps even use `const`.
> > 
> > ```
> > const Resources sum = r1 + r2;
> > const Resources diff = r1 - r2;
> > ```
> > 
> > In the following lines it becomes unclear what each variable means at a 
> > given moment anymore...

Not reusing the variable, but did not move to `const Resources` to be in sync 
with the existing tests.


> On July 8, 2016, 4:30 p.m., Jiang Yan Xu wrote:
> > src/tests/resources_tests.cpp, line 2599
> > 
> >
> > How about subtractions and contains? Seems like 
> > ScalarNonEqualSharedResources and ScalarSharedNonSharedOperations should be 
> > pretty much the same with the only difference being `isShared = true` in 
> > one `createDiskResource` call?

I kept the tests separate since `ScalarSharedNonEqualSharedOperations` tests 
shared count for shared resources when the scalar representation of shared 
resources vary (such as in disk size). `ScalarSharedNonSharedOperations` is 
about adding/subtracting 2 persistent volumes which are same in all aspects 
except their sharedness.


- Anindya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/#review141176
---


On June 13, 2016, 7:18 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated June 13, 2016, 7:18 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.
> 
> Note that v1 changes for shared resources are in the next commit.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/master/validation.cpp 7b9c2281b2ab1295211f0dd385cd77947fbd63be 
>   src/tests/mesos.hpp e9361a65eb31cced99e9ed32fd18a65d1bda26e3 
>   src/tests/resources_tests.cpp dc12bd8f1e2da6972bc8aed598811c55d664036e 
> 
> 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
> 
>

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/#review141176
---



Code LGTM. :) Comments mainly on tests.


include/mesos/resources.hpp (line 72)


For implicit converting constructors it's the convention to add an 
"annotation".

```
/*implicit*/ Resource_(const Resource& _resource)
```



include/mesos/resources.hpp (line 99)


s/adjusted/adjusted or compared/ ?



src/common/resources.cpp (line 856)


Align the two like this:

```
return sharedCount.get() >= that.sharedCount.get() &&
   resource == that.resource;
```



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


I commented on this previously:

If the substraction results in empty or invalid `Resource_`, it's 
`Resources::operator-=`'s responsibility to clean it up.

In other words, It's perfectly fine to have "cpus:100<0>" or "cpus:100<-1>" 
by themselves the same way it's fine to have "cpus:-1" by itself. It's 
`Resources` that doesn't allow them in it.

Therefore we can remove this from here.



src/common/resources.cpp (line 909)


Not *contained* here. How about something like *cannot be equal*?



src/common/resources.cpp (line 914)


s/a shared resource/shared resources/ 
s/of these resources// (because of redundancy)

?



src/tests/mesos.hpp (lines 594 - 597)


Put this in the if block above?



src/tests/resources_tests.cpp (line 2457)


A general comment is that I see this test (and the one above) mirrors 
`ScalarSubtraction` (and `ScalarAddition`) but the code duplication is 
unfortunate. Some duplication is justified beacuse we are verifying nonshared 
resource arithmetic operations still work in `Resources` when shared resources 
are present. However let's try to minimize the duplication and length the these 
tests.



src/tests/resources_tests.cpp (lines 2459 - 2493)


The test tends to get pretty long due to these constructions. Utimately it 
looks like we are just constructing two `Resources` `r1` and `r2` and testing 
their `diff`. How about simply:

```
Resource disk = createDiskResource("8192", "role1", "1", "path", None(), 
true);

Resource r1 = Resources::parse("cpus:40;mem:4096").get() + disk + disk;
Resource r2 = Resources::parse("cpus:5;mem:512").get() + disk;

Resources diff1 = r1 - r2;
```

Note that we are focusing on subtraction here.



src/tests/resources_tests.cpp (lines 2468 - 2474)


Why add them up since we are testing substraction? Can we construct r1 
directly?



src/tests/resources_tests.cpp (lines 2502 - 2507)


Let's not reuse `diff1` (it's compound assignment and not a `diff` 
anymore). If we want to verify that `-=` is equivalent to `-` we can:

```
Resources r = r1;
r1 -= r2;

// The same expectations as those of diff1.
```



src/tests/resources_tests.cpp (lines 2509 - 2524)


So what do these line do? Looks like we are interested in testing the 
situation where a shared resource is fully removed from `Resources` after 
substraction. So let's add a comment to state the goal and test it explicitly. 
Also after verifying basic `-` and `-=` already we can keep the additional 
expecatations concise.

How about:

```
// Verify that the shared disk is removed from Resources after all copies 
are substracted.
// r1 has two copies of 'disk' and r2 has one.
EXPECT_EQ(0, (r1 - r2 - r2).count(disk));
EXPECT_EQ(0, (r2 - r1).count(disk));
```



src/tests/resources_tests.cpp (line 2528)


The test is a bit long and difficult to follow and it seems to cover a lot 
of things already covered in the two tests above.

About `MultipleConsumers`: the tests above already use Resources objects 
with multiple copies of the same shared resources so we don't need redo it here.

I suggest for this test we focus on more complex expressions such as 
`EXPECT_EQ(r1 + r1 - r1, r1)` and additional methods such as `contains()`.

In order to shorten and simplify the test:

- Inline the variable constructions wherever possible.
- Don't reuse and modify 

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/#review137070
---




include/mesos/resources.hpp (line 121)


Since this is a private sub class within Resources, the data memebers are 
private in essence. Nevertheless, I moved the data meembers to be private.


- Anindya Sinha


On June 13, 2016, 7:18 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated June 13, 2016, 7:18 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.
> 
> Note that v1 changes for shared resources are in the next commit.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/master/validation.cpp 7b9c2281b2ab1295211f0dd385cd77947fbd63be 
>   src/tests/mesos.hpp e9361a65eb31cced99e9ed32fd18a65d1bda26e3 
>   src/tests/resources_tests.cpp dc12bd8f1e2da6972bc8aed598811c55d664036e 
> 
> 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
> 
>

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/
---

(Updated June 13, 2016, 7:18 a.m.)


Review request for mesos, Benjamin Mahler, Joris Van Remoortere, and Jiang Yan 
Xu.


Changes
---

Some minor cleanup and added more tests.


Bugs: MESOS-4892
https://issues.apache.org/jira/browse/MESOS-4892


Repository: mesos


Description (updated)
---

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.

Note that v1 changes for shared resources are in the next commit.


Diffs (updated)
-

  include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
  src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
  src/master/validation.cpp 7b9c2281b2ab1295211f0dd385cd77947fbd63be 
  src/tests/mesos.hpp e9361a65eb31cced99e9ed32fd18a65d1bda26e3 
  src/tests/resources_tests.cpp dc12bd8f1e2da6972bc8aed598811c55d664036e 

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

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

> On June 10, 2016, 12:10 a.m., Jiang Yan Xu wrote:
> > I feel good about the overall structure of this change with this revision. 
> > I hope I have captured most things so if we do another pass after it should 
> > mostly be relatively minor readability/reusablity/documentation/cleanups 
> > etc. which is hard to do in one go due to the length. 
> > 
> > ## About v1 resources files
> > In retropsect I feel like it probably works the better if it's in a 
> > separate patch done **after the v0 files are shipped** to avoid having them 
> > go through all intermediate revisions. i.e., you can update v1 files 
> > according to the final version of the v0 files.
> > 
> > I have mostly skipped v1 files in this review because the comments would 
> > have been duplicated. Since we are still iterating on this, would you mind 
> > pulling them out from this review?
> > 
> > ## Validation
> > If we currently only allow persistent volumes to be shared, should we 
> > validate this here?
> > 
> > ## Additional test ideas
> > - Resources A + A - A == A with shared resources in it. 
> > - Resources B + A - A == B with shared resources in A (empty resources are 
> > discarded)
> > - Resources r1 = A + A, r2 = A + A + A, r1 - r2 is empty (invalid entries 
> > are discarded)
> > - Resources r1 + A == r1 (A is an invalid shared resource) (invalid entries 
> > are discarded)
> > - Shared resources that slightly differ are not grouped together.
> > - Shared resources stream output.
> > - Resources contains() with shared resources.
> > - Any existing resources tests interesting when shared resources are 
> > blended in?
> 
> Jiang Yan Xu wrote:
> About validation: sorry I didn't realize it's in a later review /r/45960/ 
> but if you just look at this review one might wonder if you should also test 
> shared cpus or other fungible resources. Anyways, it's fine if we don't since 
> it's not a valid use case for now.

Moved v1 changes to https://reviews.apache.org/r/48616/, and added more tests.


> On June 10, 2016, 12:10 a.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 64
> > 
> >
> > We have a private section below already. Put `Resource_` definition 
> > there?

`Resource_` needs to be defined before it is used. Since we use it in the 
public section, it is defined prior to that and hence the additional `private` 
section in order to avoid moving the original `private` section of `Resources`.


> On June 10, 2016, 12:10 a.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 454-455
> > 
> >
> > Doesn't `Resource_` already friend the << operator?

This is because `Resource_` is a private member of `mesos::Resources`. Note 
this non-member global function has `const Resources::Resource_& resource` as 
its argument.


> On June 10, 2016, 12:10 a.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, line 864
> > 
> >
> > This comment still has "contained" in it.
> > 
> > How about:
> > 
> > ```
> > // The sharedness needs to match for two equal Resources_ objects.
> > ```

Updated the comment as:
`// Both Resource_ objects need to shared resources, or both need to be 
non-shared resources.`


> On June 10, 2016, 12:10 a.m., Jiang Yan Xu wrote:
> > src/tests/resources_tests.cpp, line 2534
> > 
> >
> > How is this test different than `ScalarAdditionShared` test?
> > 
> > `ScalarAdditionShared` also has multiple copies of the same shared disk 
> > added together which results in `EXPECT_EQ(2, r.count(disk1));`.

Modified `ScalarMultipleConsumers` such that it exhibits arithmetic operations 
involving shared count > 1. `ScalarAdditionShared` deals with adding with 
shared count of 1.


> On June 10, 2016, 12:10 a.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 86-87
> > 
> >
> > There's a pattern worth explaining so we can put `contains` together 
> > with the operators and have a common comment block.
> > 
> > ```
> > The Resource_ arithmetic operators and equality/contains comparison 
> > require the wrapped protobuf `resource` to have the same sharedness. 
> > 
> > For shared resources, their protobuf `resource` fields need to be the 
> > same (operators and comparisons apply to the counters). Otherwise the 
> > result is as though the arithmetic operators are not applied or 'false' for 
> > equality/contains comparison.
> > 
> > For non-shared resources the counters are none and the semantics of the 
> > Resource_ operators/comparison are the same as Resource's. See comments 
> > above the Resource operators.
> > ```
> > 
> > 

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Jiang Yan Xu]

> On June 9, 2016, 5:10 p.m., Jiang Yan Xu wrote:
> > I feel good about the overall structure of this change with this revision. 
> > I hope I have captured most things so if we do another pass after it should 
> > mostly be relatively minor readability/reusablity/documentation/cleanups 
> > etc. which is hard to do in one go due to the length. 
> > 
> > ## About v1 resources files
> > In retropsect I feel like it probably works the better if it's in a 
> > separate patch done **after the v0 files are shipped** to avoid having them 
> > go through all intermediate revisions. i.e., you can update v1 files 
> > according to the final version of the v0 files.
> > 
> > I have mostly skipped v1 files in this review because the comments would 
> > have been duplicated. Since we are still iterating on this, would you mind 
> > pulling them out from this review?
> > 
> > ## Validation
> > If we currently only allow persistent volumes to be shared, should we 
> > validate this here?
> > 
> > ## Additional test ideas
> > - Resources A + A - A == A with shared resources in it. 
> > - Resources B + A - A == B with shared resources in A (empty resources are 
> > discarded)
> > - Resources r1 = A + A, r2 = A + A + A, r1 - r2 is empty (invalid entries 
> > are discarded)
> > - Resources r1 + A == r1 (A is an invalid shared resource) (invalid entries 
> > are discarded)
> > - Shared resources that slightly differ are not grouped together.
> > - Shared resources stream output.
> > - Resources contains() with shared resources.
> > - Any existing resources tests interesting when shared resources are 
> > blended in?

About validation: sorry I didn't realize it's in a later review /r/45960/ but 
if you just look at this review one might wonder if you should also test shared 
cpus or other fungible resources. Anyways, it's fine if we don't since it's not 
a valid use case for now.


- Jiang Yan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/#review136417
---


On May 22, 2016, 12: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, 12: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
> 
>

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/#review136417
---



I feel good about the overall structure of this change with this revision. I 
hope I have captured most things so if we do another pass after it should 
mostly be relatively minor readability/reusablity/documentation/cleanups etc. 
which is hard to do in one go due to the length. 

## About v1 resources files
In retropsect I feel like it probably works the better if it's in a separate 
patch done **after the v0 files are shipped** to avoid having them go through 
all intermediate revisions. i.e., you can update v1 files according to the 
final version of the v0 files.

I have mostly skipped v1 files in this review because the comments would have 
been duplicated. Since we are still iterating on this, would you mind pulling 
them out from this review?

## Validation
If we currently only allow persistent volumes to be shared, should we validate 
this here?

## Additional test ideas
- Resources A + A - A == A with shared resources in it. 
- Resources B + A - A == B with shared resources in A (empty resources are 
discarded)
- Resources r1 = A + A, r2 = A + A + A, r1 - r2 is empty (invalid entries are 
discarded)
- Resources r1 + A == r1 (A is an invalid shared resource) (invalid entries are 
discarded)
- Shared resources that slightly differ are not grouped together.
- Shared resources stream output.
- Resources contains() with shared resources.
- Any existing resources tests interesting when shared resources are blended in?


include/mesos/resources.hpp (line 64)


We have a private section below already. Put `Resource_` definition there?



include/mesos/resources.hpp (line 76)


Instead of stating what the line does literally, add a bit of explanation:

// Setting the counter to 1 to indicate "one copy" of the shared resource.



include/mesos/resources.hpp (lines 86 - 87)


There's a pattern worth explaining so we can put `contains` together with 
the operators and have a common comment block.

```
The Resource_ arithmetic operators and equality/contains comparison require 
the wrapped protobuf `resource` to have the same sharedness. 

For shared resources, their protobuf `resource` fields need to be the same 
(operators and comparisons apply to the counters). Otherwise the result is as 
though the arithmetic operators are not applied or 'false' for 
equality/contains comparison.

For non-shared resources the counters are none and the semantics of the 
Resource_ operators/comparison are the same as Resource's. See comments above 
the Resource operators.
```

Then I feel the comments for the operators and contains in `Resources_` are 
not necessary anymore. I feel either the comment is too trivial or the comments 
are too duplicated if on individual operators/methods. What do you think?



include/mesos/resources.hpp (line 116)


s/The share count for the protobuf Resource object./The counter for 
grouping shared 'resource' objects, None if 'resource' is non-shared./



include/mesos/resources.hpp (line 121)


Since we friend the outter class. Make data members private?



include/mesos/resources.hpp (lines 125 - 126)


One blank line here.



include/mesos/resources.hpp (lines 283 - 285)


Can we avoid explaining sharedCount or share count in the public method 
because it's an internal detail?

```
Count the Resource objects that match the specified value.

NOTE:
- For a non-shared resource the count can be at most 1 because all 
non-shared Resource objects in Resources are unique.
- For a shared resource the count can be greater than 1.
```



include/mesos/resources.hpp (lines 449 - 450)


Doesn't `Resource_` already friend the << operator?



include/mesos/resources.hpp (lines 451 - 452)


Why this `friend`? This operator is already defined and we are not changing 
it right?



src/common/resources.cpp (line 826)


How about "Invalid shared resource: count < 0"?



src/common/resources.cpp (line 843)


So if (this, that) = (cpus:5<3>, cpus:4<2>), should `this` contain `that`?
It probably shouldn't?

```
// The sharedness needs to match for two Resource_ 
// objects to be contained 

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

> On May 16, 2016, 6:01 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 190
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > ```
> > 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
> > 
> >
> > 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
> > 
> >
> > `*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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > When do people need to using the vector explicitly?

Removed this.


> On May 16, 

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

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


Changes
---

Changes based on `ShareInfo` renamed to `SharedInfo`.
Cleaned up the `Resources` and `Resource_` classes.
Share count keeps track of shared resource utilization in usedResources, but it 
keeps track of whether shared resource is used or not in offer, allocator and 
sorter.


Bugs: MESOS-4892
https://issues.apache.org/jira/browse/MESOS-4892


Repository: mesos


Description (updated)
---

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 (updated)
-

  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

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Jiang Yan Xu]

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


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 
[set::count()](http://en.cppreference.com/w/cpp/container/unordered_multiset/count).



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

with 

```
bool Resources::_contains(const Resource_& that) const
```

?



src/common/resources.cpp (line 210)

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

> On April 9, 2016, 8:48 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, lines 296-304
> > 
> >
> > Why not return false if the resource type is not `SCALAR` because only 
> > `SCALAR` was supportted for now

Refactored this to use == operator for Resource.


> On April 9, 2016, 8:48 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 892
> > 
> >
> > So for such case, do you mean that a shareable resource with 3 cpus can 
> > contain a shareable resource with 5 cpus?

contains() now takes for share count for resources into consideration. Say SR 
is a share resource. Now:
SR(shareCount=5).contains(SR(shareCount=3)) = true.


> On April 9, 2016, 8:48 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, lines 1430-1431
> > 
> >
> > void Resources::_addConsumers(
> > const Resource& resource,
> > Option num_consumers)

This function was removed.


> On April 9, 2016, 8:48 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, lines 1474-1476
> > 
> >
> > void Resources::_replaceConsumers(
> > const Resource& from,
> > const Resource& to,
> > Option consumers)

This function was removed.


> On April 9, 2016, 8:48 a.m., Guangya Liu wrote:
> > src/tests/resources_tests.cpp, line 2409
> > 
> >
> > Can you please add more test cases for two resources with consumer 
> > greater than 1?
> > 
> > Ditto for the subtracble case.

Added a new task case 'ScalarMultipleConsumers' by moving it from 
https://reviews.apache.org/r/45964.


- Anindya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/#review127964
---


On April 29, 2016, 12:15 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated April 29, 2016, 12:15 a.m.)
> 
> 
> Review request for mesos, Ben 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
> 
>

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

> On April 18, 2016, 6:09 a.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 56
> > 
> >
> > 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.

Done, and moved this class within Resources.


> On April 18, 2016, 6:09 a.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 65
> > 
> >
> > 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);
> > ```

share Count is only updated within the class, and is not available to be passed 
in through a public constructor/API.


> On April 18, 2016, 6:09 a.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 348-353
> > 
> >
> > We don't need them if we stick to the "let `Resource_` take care of its 
> > own arithmetic" approach.

Removed getConsumerCount() and achieved similar semantics via the isInitState() 
api. We do not expose shareCount via a public api now.


> On April 18, 2016, 6:09 a.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 477
> > 
> >
> > If we don't need ordering (I don't think we do) then std::list is 
> > cheaper because of vector's memory allocation.

We decided to keep this a vector to avoid memory allocation / deallocation on 
every add / remove from vector.
vector does have a costly remove operation when done in the middle which was 
addressed by swapping the entry to be deleted with the last entry and removing 
the last entry.


> On April 18, 2016, 6:09 a.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, lines 1259-1286
> > 
> >
> > No need for these.

Replaced getConsumerCount() with isInitResource() to achieve the intent without 
exposing share count.


> On April 18, 2016, 6:09 a.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, lines 1259-1286
> > 
> >
> > No need for this.

Same comment as the last one. Killing this one.


> On April 18, 2016, 6:09 a.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, lines 1855-1857
> > 
> >
> > Don't add anything special for nonshared resource since it's optional.

This is not for non-shared resources. This was for shared resources when share 
count is None(). That case is also not possible so I removed this.


- Anindya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/#review128700
---


On April 29, 2016, 12:15 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated April 29, 2016, 12:15 a.m.)
> 
> 
> Review request for mesos, Ben 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 
>   

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/
---

(Updated April 29, 2016, 12:15 a.m.)


Review request for mesos, Ben Mahler, Joris Van Remoortere, and Jiang Yan Xu.


Changes
---

Refactored to not expose shareCount within `Resource_`.


Bugs: MESOS-4892
https://issues.apache.org/jira/browse/MESOS-4892


Repository: mesos


Description (updated)
---

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 (updated)
-

  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

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Jiang Yan Xu]

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


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)


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)


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)


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)


No need to define this.



include/mesos/resources.hpp (line 79)


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 

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

> On April 9, 2016, 9:02 a.m., Guangya Liu wrote:
> > You may want to rebase this patch for dynamic_reservation_framework.cpp as 
> > well.

Yes, done.


- Anindya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/#review127969
---


On April 12, 2016, 5 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated April 12, 2016, 5 a.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
> 
>

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/
---

(Updated April 12, 2016, 5 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

rebased to current master.


Bugs: MESOS-4892
https://issues.apache.org/jira/browse/MESOS-4892


Repository: mesos


Description (updated)
---

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 (updated)
-

  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

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/#review127969
---



You may want to rebase this patch for dynamic_reservation_framework.cpp as well.

- Guangya Liu


On 四月 8, 2016, 11:16 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated 四月 8, 2016, 11:16 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 df93e92252addaa794898ba95604eb2f91284b87 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   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 f781fd0102c247b2e77a71f7be82b872b0831681 
>   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
> 
>

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45959/#review127964
---




include/mesos/resources.hpp (line 64)


s/regular/non-shareable

We often user `regular` resources as `non revocable` resources, so here it 
is better use `non-shareable` to make it more clear



include/mesos/v1/resources.hpp (lines 466 - 467)


adjust the comment to make it less jagged.

// The add and subtract operators on Resource_ is only allowed
// from within Resources class, so we hide these operators.



src/common/resources.cpp (lines 296 - 304)


Why not return false if the resource type is not `SCALAR` because only 
`SCALAR` was supportted for now



src/common/resources.cpp (line 892)


So for such case, do you mean that a shareable resource with 3 cpus can 
contain a shareable resource with 5 cpus?



src/common/resources.cpp (lines 1282 - 1283)


blank line here



src/common/resources.cpp (lines 1430 - 1431)


void Resources::_addConsumers(
const Resource& resource,
Option num_consumers)



src/common/resources.cpp (lines 1474 - 1476)


void Resources::_replaceConsumers(
const Resource& from,
const Resource& to,
Option consumers)



src/tests/resources_tests.cpp (line 2409)


Can you please add more test cases for two resources with consumer greater 
than 1?

Ditto for the subtracble case.



src/tests/resources_tests.cpp (lines 2423 - 2424)


EXPECT_SOME_EQ(0, r1.getConsumerCount(disk1));

This can cover Line 2423 and Line 2424

Ditto for the following in this test: Combine `EXPECT_SOME` and `EXPECT_EQ` 
to `EXPECT_SOME_EQ`



src/tests/resources_tests.cpp (lines 2437 - 2438)


EXPECT_SOME_EQ(0, r2.getConsumerCount(disk2));



src/tests/resources_tests.cpp (lines 2449 - 2450)


EXPECT_SOME_EQ(1, sum.getConsumerCount(disk1));



src/tests/resources_tests.cpp (lines 2453 - 2454)


EXPECT_SOME_EQ(1, sum.getConsumerCount(disk2));



src/tests/resources_tests.cpp (lines 2466 - 2467)


EXPECT_SOME_EQ(1, r.getConsumerCount(disk1));



src/tests/resources_tests.cpp (lines 2470 - 2471)


EXPECT_EQ(1, r.getConsumerCount(disk2));



src/tests/resources_tests.cpp (lines 2501 - 2502)


EXPECT_SOME_EQ(1, r1.getConsumerCount(disk1));


- Guangya Liu


On 四月 8, 2016, 11:16 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated 四月 8, 2016, 11:16 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 df93e92252addaa794898ba95604eb2f91284b87 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/examples/no_executor_framework.cpp 
> f578edfd99f3b7adf19cf06eab20696532c7b67d 
>   src/examples/persistent_volume_framework.cpp 
>