Re: Review Request 70062: Refactored and augmented `class ResourceQuantities`.

[Benjamin Mahler]

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


Fix it, then Ship it!





src/common/resource_quantities.cpp
Lines 147 (patched)


Newline before the comment?



src/common/resource_quantities.cpp
Lines 152 (patched)


Newline before the if?



src/common/resource_quantities.cpp
Lines 184 (patched)


Newline before the comment?



src/common/resource_quantities.cpp
Lines 189 (patched)


Newline before the if?



src/common/resource_quantities.cpp
Lines 216 (patched)


Can you use CHECK_GE here so that it prints out the value of 'scalar' if it 
fails?


- Benjamin Mahler


On March 14, 2019, 11:53 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70062/
> ---
> 
> (Updated March 14, 2019, 11:53 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removed the map interface of
> `class ResourceQuantities`, added a few built-in
> arithmetic operations. Now, absent resource items imply
> there is no (zero) such resources.
> 
> Also added a to-do to add `class ResourceLimits` which
> is similar but treats absent resource entries as having
> infinite amount of such resource.
> 
> Also changed affected call sites and tests accordingly.
> 
> 
> Diffs
> -
> 
>   src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
>   src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
>   src/master/allocator/sorter/drf/sorter.hpp 
> e64c9ad3520a601f7854e807ef5306d5bffc0ff8 
>   src/master/allocator/sorter/drf/sorter.cpp 
> b128df08e3c93d3d1a75c637cbed359c2cb8cda4 
>   src/master/allocator/sorter/random/sorter.hpp 
> 4f230ec740e2f80d5333c61c5b23d9a631bdb273 
>   src/master/allocator/sorter/random/sorter.cpp 
> f578ef19b4dee9cf9c7c99a8988829ecde70ed6d 
>   src/tests/resource_quantities_tests.cpp 
> 435a4949b95e9a83be73781388eb4be9c7da695b 
> 
> 
> Diff: https://reviews.apache.org/r/70062/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> Dedicated tests are added in the subsequent patch.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70062: Refactored and augmented `class ResourceQuantities`.

[Meng Zhu]

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

(Updated March 14, 2019, 4:53 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Addressed Ben's comment


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


Repository: mesos


Description
---

This patch removed the map interface of
`class ResourceQuantities`, added a few built-in
arithmetic operations. Now, absent resource items imply
there is no (zero) such resources.

Also added a to-do to add `class ResourceLimits` which
is similar but treats absent resource entries as having
infinite amount of such resource.

Also changed affected call sites and tests accordingly.


Diffs (updated)
-

  src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
  src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
  src/master/allocator/sorter/drf/sorter.hpp 
e64c9ad3520a601f7854e807ef5306d5bffc0ff8 
  src/master/allocator/sorter/drf/sorter.cpp 
b128df08e3c93d3d1a75c637cbed359c2cb8cda4 
  src/master/allocator/sorter/random/sorter.hpp 
4f230ec740e2f80d5333c61c5b23d9a631bdb273 
  src/master/allocator/sorter/random/sorter.cpp 
f578ef19b4dee9cf9c7c99a8988829ecde70ed6d 
  src/tests/resource_quantities_tests.cpp 
435a4949b95e9a83be73781388eb4be9c7da695b 


Diff: https://reviews.apache.org/r/70062/diff/5/

Changes: https://reviews.apache.org/r/70062/diff/4-5/


Testing
---

make check
Dedicated tests are added in the subsequent patch.


Thanks,

Meng Zhu

Re: Review Request 70062: Refactored and augmented `class ResourceQuantities`.

[Meng Zhu]

> On March 13, 2019, 11:55 a.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.hpp
> > Line 122 (original), 114 (patched)
> > 
> >
> > I'm curious why the rename, `get` still seems like a more readable name 
> > to me:
> > 
> > ```
> > quantities.get("cpus")
> > vs
> > quantities.quantity("cpus")
> > ```
> > 
> > "quantity" doesn't seem to suggest the action of retrieving the value 
> > for the name?

ok, `get` sounds good to me.
I feel `get` by default carries the connotation of getting a key (given I know 
underlying is a map interface). But I think for typical users `get` is probably 
fine. I don't have a strong opinion here. Let's use `get` then.


> On March 13, 2019, 11:55 a.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.cpp
> > Lines 158-159 (patched)
> > 
> >
> > Why not advance left here? The current code does this AFAICT:
> > 
> > // Before:
> > //LeftIndex
> > //v
> > // Left  [0, ..., b, ...]
> > // Right [0, ..., a, ...]
> > //^ RightIndex
> > 
> > // After:
> > //LeftIndex
> > //v
> > // Left  [0, ..., a, b, ...]
> > // Right [0, ..., a, z, ...]
> > //   ^ RightIndex
> > 
> > We insert the missing entry into left, and we know that since we moved 
> > right index forward the next loop iteration will just execute ++leftIndex. 
> > So there's no correctness issue. However, it seems a bit strange that we've 
> > essentially taken one step backwards with left index and are now pointing 
> > to the item before `b`.
> > 
> > Logically, it seems a bit clearer to increment left since we want to 
> > stay indexed on `b` for the next iteration?
> > 
> > // Before:
> > //LeftIndex
> > //v
> > // Left  [0, ..., b, ...]
> > // Right [0, ..., a, ...]
> > //^ RightIndex
> > 
> > // After:
> > //   LeftIndex
> > //   v
> > // Left  [0, ..., a, b, ...]
> > // Right [0, ..., a, z, ...]
> > //   ^ RightIndex

Yeah, let's advance here. Thanks!


> On March 13, 2019, 11:55 a.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.cpp
> > Lines 168-174 (patched)
> > 
> >
> > Might read a bit more consistent and succinct like this?
> > 
> > ```
> >   // Copy the remaining items in `right`.
> >   while (rightIndex < right.size()) {
> > quantities.push_back(right.quantities.at(rightIndex));
> > ++rightIndex;
> >   }
> > ```
> > 
> > Basically just "finishing" the loop above for the right index.

Sounds good.


> On March 13, 2019, 11:55 a.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.cpp
> > Lines 217 (patched)
> > 
> >
> > Any thoughts on CHECKing > 0 or >= 0 vs no CHECK?

I checked `>= 0`. Let's just drop the zero here.


- Meng


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


On March 14, 2019, 4:53 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70062/
> ---
> 
> (Updated March 14, 2019, 4:53 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removed the map interface of
> `class ResourceQuantities`, added a few built-in
> arithmetic operations. Now, absent resource items imply
> there is no (zero) such resources.
> 
> Also added a to-do to add `class ResourceLimits` which
> is similar but treats absent resource entries as having
> infinite amount of such resource.
> 
> Also changed affected call sites and tests accordingly.
> 
> 
> Diffs
> -
> 
>   src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
>   src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
>   src/master/allocator/sorter/drf/sorter.hpp 
> e64c9ad3520a601f7854e807ef5306d5bffc0ff8 
>   src/master/allocator/sorter/drf/sorter.cpp 
> b128df08e3c93d3d1a75c637cbed359c2cb8cda4 
>   src/master/allocator/sorter/random/sorter.hpp 
> 4f230ec740e2f80d5333c61c5b23d9a631bdb273 
>   src/master/allocator/sorter/random/sorter.cpp 
> f

Re: Review Request 70062: Refactored and augmented `class ResourceQuantities`.

[Benjamin Mahler]

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



Looks good, just one minor quirk about the += operator, and some stylistic 
comments.


src/common/resource_quantities.hpp
Line 122 (original), 114 (patched)


I'm curious why the rename, `get` still seems like a more readable name to 
me:

```
quantities.get("cpus")
vs
quantities.quantity("cpus")
```

"quantity" doesn't seem to suggest the action of retrieving the value for 
the name?



src/common/resource_quantities.hpp
Lines 124-125 (original), 116-117 (patched)


Up here in the public interface seems like we should name the argument 
`quantities`? We can still name using `left`/`right` or `this`/`that` in the 
.cpp



src/common/resource_quantities.cpp
Lines 145 (patched)


Don't know if style guide allows this, can you do:

```
size_t leftIndex = 0u;
size_t rightIndex = 0u;

while (leftIndex < size() && rightIndex < right.size()) {
  ...
```



src/common/resource_quantities.cpp
Lines 150-151 (patched)


This comment seems to belong up above the loop to explain the overall 
approach?



src/common/resource_quantities.cpp
Lines 158-159 (patched)


Why not advance left here? The current code does this AFAICT:

// Before:
//LeftIndex
//v
// Left  [0, ..., b, ...]
// Right [0, ..., a, ...]
//^ RightIndex

// After:
//LeftIndex
//v
// Left  [0, ..., a, b, ...]
// Right [0, ..., a, z, ...]
//   ^ RightIndex

We insert the missing entry into left, and we know that since we moved 
right index forward the next loop iteration will just execute ++leftIndex. So 
there's no correctness issue. However, it seems a bit strange that we've 
essentially taken one step backwards with left index and are now pointing to 
the item before `b`.

Logically, it seems a bit clearer to increment left since we want to stay 
indexed on `b` for the next iteration?

// Before:
//LeftIndex
//v
// Left  [0, ..., b, ...]
// Right [0, ..., a, ...]
//^ RightIndex

// After:
//   LeftIndex
//   v
// Left  [0, ..., a, b, ...]
// Right [0, ..., a, z, ...]
//   ^ RightIndex



src/common/resource_quantities.cpp
Lines 168-174 (patched)


Might read a bit more consistent and succinct like this?

```
  // Copy the remaining items in `right`.
  while (rightIndex < right.size()) {
quantities.push_back(right.quantities.at(rightIndex));
++rightIndex;
  }
```

Basically just "finishing" the loop above for the right index.



src/common/resource_quantities.cpp
Lines 183-184 (patched)


Maybe just pull these out and have a more consistent while loop style to 
the += operator?

```
size_t leftIndex = 0u;
size_t rightIndex = 0u;

while (leftIndex < size() &&  rightIndex < right.size()) {
  ...
```

Seems a bit "cleaner" looking too?



src/common/resource_quantities.cpp
Lines 188-189 (patched)


Ditto here, seems to belong above the loop?



src/common/resource_quantities.cpp
Lines 194-195 (patched)


A little unclear that this is because it's negative?

```
  // Item exists in the right but not in the left (i.e. 0), this
  // would result in a negative entry, so skip it.
```



src/common/resource_quantities.cpp
Lines 217 (patched)


Any thoughts on CHECKing > 0 or >= 0 vs no CHECK?


- Benjamin Mahler


On March 12, 2019, 11:17 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70062/
> ---
> 
> (Updated March 12, 2019, 11:17 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removed the map interface of
> `class ResourceQuantities`, added a few built-in

Re: Review Request 70062: Refactored and augmented `class ResourceQuantities`.

[Meng Zhu]

> On March 11, 2019, 3:28 p.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.cpp
> > Lines 143 (patched)
> > 
> >
> > naming wise, perhaps 'this' and 'other' / 'otherIndex' or 'left' and 
> > 'right' / 'rightIndex' for these functions?

Done.


- Meng


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


On March 12, 2019, 4:17 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70062/
> ---
> 
> (Updated March 12, 2019, 4:17 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removed the map interface of
> `class ResourceQuantities`, added a few built-in
> arithmetic operations. Now, absent resource items imply
> there is no (zero) such resources.
> 
> Also added a to-do to add `class ResourceLimits` which
> is similar but treats absent resource entries as having
> infinite amount of such resource.
> 
> Also changed affected call sites and tests accordingly.
> 
> 
> Diffs
> -
> 
>   src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
>   src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
>   src/master/allocator/sorter/drf/sorter.hpp 
> e64c9ad3520a601f7854e807ef5306d5bffc0ff8 
>   src/master/allocator/sorter/drf/sorter.cpp 
> b128df08e3c93d3d1a75c637cbed359c2cb8cda4 
>   src/master/allocator/sorter/random/sorter.hpp 
> 4f230ec740e2f80d5333c61c5b23d9a631bdb273 
>   src/master/allocator/sorter/random/sorter.cpp 
> f578ef19b4dee9cf9c7c99a8988829ecde70ed6d 
>   src/tests/resource_quantities_tests.cpp 
> 435a4949b95e9a83be73781388eb4be9c7da695b 
> 
> 
> Diff: https://reviews.apache.org/r/70062/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> Dedicated tests are added in the subsequent patch.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70062: Refactored and augmented `class ResourceQuantities`.

[Meng Zhu]

> On March 11, 2019, 3:35 p.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.cpp
> > Lines 153 (patched)
> > 
> >
> > Maybe some one-liner comments on these conditions to help the reader? 
> > E.g.
> > 
> > // Right contains item not in left.

Done.


- Meng


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


On March 12, 2019, 4:17 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70062/
> ---
> 
> (Updated March 12, 2019, 4:17 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removed the map interface of
> `class ResourceQuantities`, added a few built-in
> arithmetic operations. Now, absent resource items imply
> there is no (zero) such resources.
> 
> Also added a to-do to add `class ResourceLimits` which
> is similar but treats absent resource entries as having
> infinite amount of such resource.
> 
> Also changed affected call sites and tests accordingly.
> 
> 
> Diffs
> -
> 
>   src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
>   src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
>   src/master/allocator/sorter/drf/sorter.hpp 
> e64c9ad3520a601f7854e807ef5306d5bffc0ff8 
>   src/master/allocator/sorter/drf/sorter.cpp 
> b128df08e3c93d3d1a75c637cbed359c2cb8cda4 
>   src/master/allocator/sorter/random/sorter.hpp 
> 4f230ec740e2f80d5333c61c5b23d9a631bdb273 
>   src/master/allocator/sorter/random/sorter.cpp 
> f578ef19b4dee9cf9c7c99a8988829ecde70ed6d 
>   src/tests/resource_quantities_tests.cpp 
> 435a4949b95e9a83be73781388eb4be9c7da695b 
> 
> 
> Diff: https://reviews.apache.org/r/70062/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> Dedicated tests are added in the subsequent patch.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70062: Refactored and augmented `class ResourceQuantities`.

[Meng Zhu]

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

(Updated March 12, 2019, 4:17 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Addressed Ben's comment.


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


Repository: mesos


Description
---

This patch removed the map interface of
`class ResourceQuantities`, added a few built-in
arithmetic operations. Now, absent resource items imply
there is no (zero) such resources.

Also added a to-do to add `class ResourceLimits` which
is similar but treats absent resource entries as having
infinite amount of such resource.

Also changed affected call sites and tests accordingly.


Diffs (updated)
-

  src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
  src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
  src/master/allocator/sorter/drf/sorter.hpp 
e64c9ad3520a601f7854e807ef5306d5bffc0ff8 
  src/master/allocator/sorter/drf/sorter.cpp 
b128df08e3c93d3d1a75c637cbed359c2cb8cda4 
  src/master/allocator/sorter/random/sorter.hpp 
4f230ec740e2f80d5333c61c5b23d9a631bdb273 
  src/master/allocator/sorter/random/sorter.cpp 
f578ef19b4dee9cf9c7c99a8988829ecde70ed6d 
  src/tests/resource_quantities_tests.cpp 
435a4949b95e9a83be73781388eb4be9c7da695b 


Diff: https://reviews.apache.org/r/70062/diff/4/

Changes: https://reviews.apache.org/r/70062/diff/3-4/


Testing
---

make check
Dedicated tests are added in the subsequent patch.


Thanks,

Meng Zhu

Re: Review Request 70062: Refactored and augmented `class ResourceQuantities`.

[Meng Zhu]

> On March 11, 2019, 3:15 p.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.hpp
> > Lines 109-111 (original), 112-115 (patched)
> > 
> >
> > Do we still need this now that we're splitting the class? Maybe remove 
> > it for now or make it return non-option `Value::Scalar` and None becomes 0?

yep, still needed e.g. 
https://github.com/apache/mesos/blob/0dc24dba9afdca1948eea5cad05f861162dc8dd6/src/master/allocator/sorter/drf/sorter.cpp#L675
I made it non-option and `None` becomes 0.


- Meng


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


On March 12, 2019, 4:17 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70062/
> ---
> 
> (Updated March 12, 2019, 4:17 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removed the map interface of
> `class ResourceQuantities`, added a few built-in
> arithmetic operations. Now, absent resource items imply
> there is no (zero) such resources.
> 
> Also added a to-do to add `class ResourceLimits` which
> is similar but treats absent resource entries as having
> infinite amount of such resource.
> 
> Also changed affected call sites and tests accordingly.
> 
> 
> Diffs
> -
> 
>   src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
>   src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
>   src/master/allocator/sorter/drf/sorter.hpp 
> e64c9ad3520a601f7854e807ef5306d5bffc0ff8 
>   src/master/allocator/sorter/drf/sorter.cpp 
> b128df08e3c93d3d1a75c637cbed359c2cb8cda4 
>   src/master/allocator/sorter/random/sorter.hpp 
> 4f230ec740e2f80d5333c61c5b23d9a631bdb273 
>   src/master/allocator/sorter/random/sorter.cpp 
> f578ef19b4dee9cf9c7c99a8988829ecde70ed6d 
>   src/tests/resource_quantities_tests.cpp 
> 435a4949b95e9a83be73781388eb4be9c7da695b 
> 
> 
> Diff: https://reviews.apache.org/r/70062/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> Dedicated tests are added in the subsequent patch.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70062: Refactored and augmented `class ResourceQuantities`.

[Benjamin Mahler]

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




src/common/resource_quantities.cpp
Lines 153 (patched)


Maybe some one-liner comments on these conditions to help the reader? E.g.

// Right contains item not in left.


- Benjamin Mahler


On March 7, 2019, 2:35 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70062/
> ---
> 
> (Updated March 7, 2019, 2:35 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removed the map interface of
> `class ResourceQuantities`, added a few built-in
> arithmetic operations. Now, absent resource items imply
> there is no (zero) such resources.
> 
> Also added a to-do to add `class ResourceLimits` which
> is similar but treats absent resource entries as having
> infinite amount of such resource.
> 
> Also changed affected call sites and tests accordingly.
> 
> 
> Diffs
> -
> 
>   src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
>   src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
>   src/master/allocator/sorter/drf/sorter.hpp 
> e64c9ad3520a601f7854e807ef5306d5bffc0ff8 
>   src/master/allocator/sorter/drf/sorter.cpp 
> b128df08e3c93d3d1a75c637cbed359c2cb8cda4 
>   src/master/allocator/sorter/random/sorter.hpp 
> 4f230ec740e2f80d5333c61c5b23d9a631bdb273 
>   src/master/allocator/sorter/random/sorter.cpp 
> f578ef19b4dee9cf9c7c99a8988829ecde70ed6d 
>   src/tests/resource_quantities_tests.cpp 
> 435a4949b95e9a83be73781388eb4be9c7da695b 
> 
> 
> Diff: https://reviews.apache.org/r/70062/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> Dedicated tests are added in the subsequent patch.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70062: Refactored and augmented `class ResourceQuantities`.

[Benjamin Mahler]

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




src/common/resource_quantities.hpp
Lines 109-111 (original), 112-115 (patched)


Do we still need this now that we're splitting the class? Maybe remove it 
for now or make it return non-option `Value::Scalar` and None becomes 0?



src/common/resource_quantities.cpp
Lines 155 (patched)


Looks like this insertion may invalidate the iterator, we could write these 
loops using indexes instead of iterators?



src/common/resource_quantities.cpp
Lines 189 (patched)


zero and negative?



src/common/resource_quantities.cpp
Lines 190 (patched)


Ditto here, let's use indexes?



src/common/resource_quantities.cpp
Line 141 (original), 204 (patched)


Maybe we could have an `add(name, value)` function that replaces this to 
avoid the brittleness of `[]` leaving a 0 entry and relying on the caller to 
increment it out of the 0 state?


- Benjamin Mahler


On March 7, 2019, 2:35 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70062/
> ---
> 
> (Updated March 7, 2019, 2:35 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removed the map interface of
> `class ResourceQuantities`, added a few built-in
> arithmetic operations. Now, absent resource items imply
> there is no (zero) such resources.
> 
> Also added a to-do to add `class ResourceLimits` which
> is similar but treats absent resource entries as having
> infinite amount of such resource.
> 
> Also changed affected call sites and tests accordingly.
> 
> 
> Diffs
> -
> 
>   src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
>   src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
>   src/master/allocator/sorter/drf/sorter.hpp 
> e64c9ad3520a601f7854e807ef5306d5bffc0ff8 
>   src/master/allocator/sorter/drf/sorter.cpp 
> b128df08e3c93d3d1a75c637cbed359c2cb8cda4 
>   src/master/allocator/sorter/random/sorter.hpp 
> 4f230ec740e2f80d5333c61c5b23d9a631bdb273 
>   src/master/allocator/sorter/random/sorter.cpp 
> f578ef19b4dee9cf9c7c99a8988829ecde70ed6d 
>   src/tests/resource_quantities_tests.cpp 
> 435a4949b95e9a83be73781388eb4be9c7da695b 
> 
> 
> Diff: https://reviews.apache.org/r/70062/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> Dedicated tests are added in the subsequent patch.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70062: Refactored and augmented `class ResourceQuantities`.

[Benjamin Mahler]

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




src/common/resource_quantities.cpp
Lines 143 (patched)


naming wise, perhaps 'this' and 'other' / 'otherIndex' or 'left' and 
'right' / 'rightIndex' for these functions?


- Benjamin Mahler


On March 7, 2019, 2:35 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70062/
> ---
> 
> (Updated March 7, 2019, 2:35 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removed the map interface of
> `class ResourceQuantities`, added a few built-in
> arithmetic operations. Now, absent resource items imply
> there is no (zero) such resources.
> 
> Also added a to-do to add `class ResourceLimits` which
> is similar but treats absent resource entries as having
> infinite amount of such resource.
> 
> Also changed affected call sites and tests accordingly.
> 
> 
> Diffs
> -
> 
>   src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
>   src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
>   src/master/allocator/sorter/drf/sorter.hpp 
> e64c9ad3520a601f7854e807ef5306d5bffc0ff8 
>   src/master/allocator/sorter/drf/sorter.cpp 
> b128df08e3c93d3d1a75c637cbed359c2cb8cda4 
>   src/master/allocator/sorter/random/sorter.hpp 
> 4f230ec740e2f80d5333c61c5b23d9a631bdb273 
>   src/master/allocator/sorter/random/sorter.cpp 
> f578ef19b4dee9cf9c7c99a8988829ecde70ed6d 
>   src/tests/resource_quantities_tests.cpp 
> 435a4949b95e9a83be73781388eb4be9c7da695b 
> 
> 
> Diff: https://reviews.apache.org/r/70062/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> Dedicated tests are added in the subsequent patch.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70062: Refactored and augmented `class ResourceQuantities`.

[Meng Zhu]

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

(Updated March 6, 2019, 6:35 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Minor comment fix.


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


Repository: mesos


Description
---

This patch removed the map interface of
`class ResourceQuantities`, added a few built-in
arithmetic operations. Now, absent resource items imply
there is no (zero) such resources.

Also added a to-do to add `class ResourceLimits` which
is similar but treats absent resource entries as having
infinite amount of such resource.

Also changed affected call sites and tests accordingly.


Diffs (updated)
-

  src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
  src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
  src/master/allocator/sorter/drf/sorter.hpp 
e64c9ad3520a601f7854e807ef5306d5bffc0ff8 
  src/master/allocator/sorter/drf/sorter.cpp 
b128df08e3c93d3d1a75c637cbed359c2cb8cda4 
  src/master/allocator/sorter/random/sorter.hpp 
4f230ec740e2f80d5333c61c5b23d9a631bdb273 
  src/master/allocator/sorter/random/sorter.cpp 
f578ef19b4dee9cf9c7c99a8988829ecde70ed6d 
  src/tests/resource_quantities_tests.cpp 
435a4949b95e9a83be73781388eb4be9c7da695b 


Diff: https://reviews.apache.org/r/70062/diff/3/

Changes: https://reviews.apache.org/r/70062/diff/2-3/


Testing
---

make check
Dedicated tests are added in the subsequent patch.


Thanks,

Meng Zhu

Re: Review Request 70062: Refactored and augmented `class ResourceQuantities`.

[Meng Zhu]

> On Feb. 27, 2019, 9:04 a.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.cpp
> > Lines 61-64 (original), 61-64 (patched)
> > 
> >
> > Ditto comment below, it seems 0 should be allowed here.

Agreed. Done.


> On Feb. 27, 2019, 9:04 a.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.cpp
> > Lines 141-178 (patched)
> > 
> >
> > Can you make these more symmetrical?
> > 
> > If we want to keep the [] operator, why don't we use it in -= and also 
> > use a new .erase function?
> > 
> > ```
> >   // TODO: Walk both vectors at the same time, rather than re-searching.
> >   foreach (auto& that, those) {
> > (*this)[that.first] -= that.second;
> > 
> > if ((*this)[that.first] == 0) {
> >   erase(that.first);
> > }
> >   }
> > 
> >   return *this;
> > ```
> > 
> > There doesn't seem to be a big benefit from the existing code in this 
> > patch for the complexity it has? It still is re-searching the list from the 
> > beginning each iteration as opposed to walking both lists assuming 
> > alphabetical ordering for better performance.

Made them symmetrical, both take advantage of the ordering and are now 
one-pass. If we just do the loop thing, it kinds of defeat the purpose of 
ordering. Anyway, these are going to be used in performance critical places, so 
a bit of performance gain for the complexity seems to be worth it.

`[]` is still kept, mainly for the (current and future) constructors and 
conversion methods.


> On Feb. 27, 2019, 9:04 a.m., Benjamin Mahler wrote:
> > src/master/allocator/sorter/drf/sorter.hpp
> > Line 347 (original), 347 (patched)
> > 
> >
> > Seems like we sould just allow an implicit constructor to make this 
> > seamless?
> > 
> > ```
> >   totals += quantitiesToAdd;
> > ```

I want to distinguish this from future conversations interfaces where 
non-scalar resources are also converted into quantities.


> On Feb. 27, 2019, 9:04 a.m., Benjamin Mahler wrote:
> > src/tests/resource_quantities_tests.cpp
> > Lines 116-118 (patched)
> > 
> >
> > Is this consistent with Resources? This seems fine and it should just 
> > be not stored.

Yep, fixed.


- Meng


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


On Feb. 26, 2019, 5:29 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70062/
> ---
> 
> (Updated Feb. 26, 2019, 5:29 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removed the map interface of
> `class ResourceQuantities`, added a few built-in
> arithmetic operations. Now, absent resource items imply
> there is no (zero) such resources.
> 
> Also added a to-do to add `class ResourceLimits` which
> is similar but treats absent resource entries as having
> infinite amount of such resource.
> 
> Also changed affected call sites and tests accordingly.
> 
> 
> Diffs
> -
> 
>   src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
>   src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
>   src/master/allocator/sorter/drf/sorter.hpp 
> e64c9ad3520a601f7854e807ef5306d5bffc0ff8 
>   src/master/allocator/sorter/drf/sorter.cpp 
> b128df08e3c93d3d1a75c637cbed359c2cb8cda4 
>   src/master/allocator/sorter/random/sorter.hpp 
> 4f230ec740e2f80d5333c61c5b23d9a631bdb273 
>   src/master/allocator/sorter/random/sorter.cpp 
> f578ef19b4dee9cf9c7c99a8988829ecde70ed6d 
>   src/tests/resource_quantities_tests.cpp 
> 435a4949b95e9a83be73781388eb4be9c7da695b 
> 
> 
> Diff: https://reviews.apache.org/r/70062/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> Dedicated tests are added in the subsequent patch.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70062: Refactored and augmented `class ResourceQuantities`.

[Benjamin Mahler]

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




src/common/resource_quantities.cpp
Lines 61-64 (original), 61-64 (patched)


Ditto comment below, it seems 0 should be allowed here.



src/common/resource_quantities.cpp
Lines 141-178 (patched)


Can you make these more symmetrical?

If we want to keep the [] operator, why don't we use it in -= and also use 
a new .erase function?

```
  // TODO: Walk both vectors at the same time, rather than re-searching.
  foreach (auto& that, those) {
(*this)[that.first] -= that.second;

if ((*this)[that.first] == 0) {
  erase(that.first);
}
  }

  return *this;
```

There doesn't seem to be a big benefit from the existing code in this patch 
for the complexity it has? It still is re-searching the list from the beginning 
each iteration as opposed to walking both lists assuming alphabetical ordering 
for better performance.



src/master/allocator/sorter/drf/sorter.hpp
Line 347 (original), 347 (patched)


Seems like we sould just allow an implicit constructor to make this 
seamless?

```
  totals += quantitiesToAdd;
```



src/tests/resource_quantities_tests.cpp
Lines 116-118 (patched)


Is this consistent with Resources? This seems fine and it should just be 
not stored.


- Benjamin Mahler


On Feb. 27, 2019, 1:29 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70062/
> ---
> 
> (Updated Feb. 27, 2019, 1:29 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removed the map interface of
> `class ResourceQuantities`, added a few built-in
> arithmetic operations. Now, absent resource items imply
> there is no (zero) such resources.
> 
> Also added a to-do to add `class ResourceLimits` which
> is similar but treats absent resource entries as having
> infinite amount of such resource.
> 
> Also changed affected call sites and tests accordingly.
> 
> 
> Diffs
> -
> 
>   src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
>   src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
>   src/master/allocator/sorter/drf/sorter.hpp 
> e64c9ad3520a601f7854e807ef5306d5bffc0ff8 
>   src/master/allocator/sorter/drf/sorter.cpp 
> b128df08e3c93d3d1a75c637cbed359c2cb8cda4 
>   src/master/allocator/sorter/random/sorter.hpp 
> 4f230ec740e2f80d5333c61c5b23d9a631bdb273 
>   src/master/allocator/sorter/random/sorter.cpp 
> f578ef19b4dee9cf9c7c99a8988829ecde70ed6d 
>   src/tests/resource_quantities_tests.cpp 
> 435a4949b95e9a83be73781388eb4be9c7da695b 
> 
> 
> Diff: https://reviews.apache.org/r/70062/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> Dedicated tests are added in the subsequent patch.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 70062: Refactored and augmented `class ResourceQuantities`.

[Meng Zhu]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This patch removed the map interface of
`class ResourceQuantities`, added a few built-in
arithmetic operations. Now, absent resource items imply
there is no (zero) such resources.

Also added a to-do to add `class ResourceLimits` which
is similar but treats absent resource entries as having
infinite amount of such resource.

Also changed affected call sites and tests accordingly.


Diffs
-

  src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
  src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
  src/master/allocator/sorter/drf/sorter.hpp 
e64c9ad3520a601f7854e807ef5306d5bffc0ff8 
  src/master/allocator/sorter/drf/sorter.cpp 
b128df08e3c93d3d1a75c637cbed359c2cb8cda4 
  src/master/allocator/sorter/random/sorter.hpp 
4f230ec740e2f80d5333c61c5b23d9a631bdb273 
  src/master/allocator/sorter/random/sorter.cpp 
f578ef19b4dee9cf9c7c99a8988829ecde70ed6d 
  src/tests/resource_quantities_tests.cpp 
435a4949b95e9a83be73781388eb4be9c7da695b 


Diff: https://reviews.apache.org/r/70062/diff/1/


Testing
---

make check
Dedicated tests are added in the subsequent patch.


Thanks,

Meng Zhu