You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Meng Zhu <mz...@mesosphere.io> on 2019/03/07 02:36:00 UTC
Re: Review Request 70062: Refactored and augmented `class
ResourceQuantities`.
-----------------------------------------------------------
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`.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On March 11, 2019, 3:15 p.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.hpp
> > Lines 109-111 (original), 112-115 (patched)
> > <https://reviews.apache.org/r/70062/diff/1-3/?file=2127048#file2127048line112>
> >
> > 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`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/70062/#comment299578>
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)
<https://reviews.apache.org/r/70062/#comment299574>
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)
<https://reviews.apache.org/r/70062/#comment299575>
zero and negative?
src/common/resource_quantities.cpp
Lines 190 (patched)
<https://reviews.apache.org/r/70062/#comment299576>
Ditto here, let's use indexes?
src/common/resource_quantities.cpp
Line 141 (original), 204 (patched)
<https://reviews.apache.org/r/70062/#comment299577>
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`.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On March 11, 2019, 3:35 p.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.cpp
> > Lines 153 (patched)
> > <https://reviews.apache.org/r/70062/diff/3/?file=2129286#file2129286line154>
> >
> > 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`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/70062/#comment299593>
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`.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On March 11, 2019, 3:28 p.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.cpp
> > Lines 143 (patched)
> > <https://reviews.apache.org/r/70062/diff/3/?file=2129286#file2129286line144>
> >
> > 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`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/70062/#comment299584>
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`.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On March 13, 2019, 11:55 a.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.hpp
> > Line 122 (original), 114 (patched)
> > <https://reviews.apache.org/r/70062/diff/4/?file=2131638#file2131638line124>
> >
> > 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)
> > <https://reviews.apache.org/r/70062/diff/4/?file=2131639#file2131639line159>
> >
> > 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)
> > <https://reviews.apache.org/r/70062/diff/4/?file=2131639#file2131639line169>
> >
> > 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)
> > <https://reviews.apache.org/r/70062/diff/4/?file=2131639#file2131639line218>
> >
> > 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 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`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/70062/#comment299657>
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)
<https://reviews.apache.org/r/70062/#comment299658>
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)
<https://reviews.apache.org/r/70062/#comment299660>
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)
<https://reviews.apache.org/r/70062/#comment299661>
This comment seems to belong up above the loop to explain the overall approach?
src/common/resource_quantities.cpp
Lines 158-159 (patched)
<https://reviews.apache.org/r/70062/#comment299662>
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)
<https://reviews.apache.org/r/70062/#comment299663>
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)
<https://reviews.apache.org/r/70062/#comment299664>
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)
<https://reviews.apache.org/r/70062/#comment299665>
Ditto here, seems to belong above the loop?
src/common/resource_quantities.cpp
Lines 194-195 (patched)
<https://reviews.apache.org/r/70062/#comment299666>
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)
<https://reviews.apache.org/r/70062/#comment299659>
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
> 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`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/70062/#comment299730>
Newline before the comment?
src/common/resource_quantities.cpp
Lines 152 (patched)
<https://reviews.apache.org/r/70062/#comment299731>
Newline before the if?
src/common/resource_quantities.cpp
Lines 184 (patched)
<https://reviews.apache.org/r/70062/#comment299732>
Newline before the comment?
src/common/resource_quantities.cpp
Lines 189 (patched)
<https://reviews.apache.org/r/70062/#comment299733>
Newline before the if?
src/common/resource_quantities.cpp
Lines 216 (patched)
<https://reviews.apache.org/r/70062/#comment299734>
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`.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
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`.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
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