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 07:01:36 UTC
Review Request 70149: Added a `contains` method in
`ResourceQuantities`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70149/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Repository: mesos
Description
-------
The method checks if the `ResourceQuantities` object
contains another `ResourceQuantities`.
Also added tests.
Diffs
-----
src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae
src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7
src/tests/resource_quantities_tests.cpp 435a4949b95e9a83be73781388eb4be9c7da695b
Diff: https://reviews.apache.org/r/70149/diff/1/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 70149: Added a `contains` method in
`ResourceQuantities`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70149/#review213675
-----------------------------------------------------------
Looks good, just some minor stylistic comments that are similar to the ones I just made in the quantities refactoring patch.
src/common/resource_quantities.cpp
Lines 144-145 (patched)
<https://reviews.apache.org/r/70149/#comment299667>
This looks a bit cleaner and we can use this loop structure consistently in all of the functions here? (see my review on the other quantities patch):
```
size_t leftIndex = 0u;
size_t rightIndex = 0u;
while (leftIndex < size() && rightIndex < right.size()) {
...
```
src/common/resource_quantities.cpp
Lines 146-147 (patched)
<https://reviews.apache.org/r/70149/#comment299668>
Whoa this confused me at first, let's also use two statements here, we don't make much use of the comma syntax for declaring multiple variables in one statement:
```
const pair<string, Value::Scalar>& left_ = quantities.at(leftIndex);
const pair<string, Value::Scalar>& right_ = right.quantities.at(rightIndex);
```
src/common/resource_quantities.cpp
Lines 149-150 (patched)
<https://reviews.apache.org/r/70149/#comment299669>
Ditto here from my other review on the quantities patch, this seems to belong above the loop?
src/common/resource_quantities.cpp
Lines 162-163 (patched)
<https://reviews.apache.org/r/70149/#comment299670>
We should stick to pre-increment instead of using post-increment here?
- Benjamin Mahler
On March 12, 2019, 11:30 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70149/
> -----------------------------------------------------------
>
> (Updated March 12, 2019, 11:30 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The method checks if the ResourceQuantities object
> contains another ResourceQuantities.
>
> Also added tests.
>
>
> Diffs
> -----
>
> src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae
> src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7
> src/tests/resource_quantities_tests.cpp 435a4949b95e9a83be73781388eb4be9c7da695b
>
>
> Diff: https://reviews.apache.org/r/70149/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 70149: Added a `contains` method in
`ResourceQuantities`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70149/#review213747
-----------------------------------------------------------
Fix it, then Ship it!
src/common/resource_quantities.cpp
Lines 146 (patched)
<https://reviews.apache.org/r/70149/#comment299735>
Newline before the comment?
src/common/resource_quantities.cpp
Lines 149-150 (patched)
<https://reviews.apache.org/r/70149/#comment299737>
Just a note that we should consider making all these loops more readable as a follow up, ideally we could unpack the pairs because the .first and .second accesses aren't easy to read.
I guess the best option is only in C++17:
https://skebanga.github.io/structured-bindings/
For now we could just do the unpacking more manually:
```
const pair<string, Value::Scalar>& left_ = quantities.at(leftIndex);
const pair<string, Value::Scalar>& right_ = right.quantities.at(rightIndex);
const string& leftName = left_.first;
const string& rightName = right_.first;
const Value::Scalar& leftValue = left_.second;
const Value::Scalar& rightName = right_.second;
if (leftName < rightName) {
...
```
src/common/resource_quantities.cpp
Lines 151 (patched)
<https://reviews.apache.org/r/70149/#comment299736>
Newline before the if?
- Benjamin Mahler
On March 14, 2019, 11:56 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70149/
> -----------------------------------------------------------
>
> (Updated March 14, 2019, 11:56 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The method checks if the ResourceQuantities object
> contains another ResourceQuantities.
>
> Also added tests.
>
>
> Diffs
> -----
>
> src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae
> src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7
> src/tests/resource_quantities_tests.cpp 435a4949b95e9a83be73781388eb4be9c7da695b
>
>
> Diff: https://reviews.apache.org/r/70149/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 70149: Added a `contains` method in
`ResourceQuantities`.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70149/
-----------------------------------------------------------
(Updated March 14, 2019, 4:56 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Addressed Ben's comment.
Repository: mesos
Description
-------
The method checks if the ResourceQuantities object
contains another ResourceQuantities.
Also added tests.
Diffs (updated)
-----
src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae
src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7
src/tests/resource_quantities_tests.cpp 435a4949b95e9a83be73781388eb4be9c7da695b
Diff: https://reviews.apache.org/r/70149/diff/3/
Changes: https://reviews.apache.org/r/70149/diff/2-3/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 70149: Added a `contains` method in
`ResourceQuantities`.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70149/
-----------------------------------------------------------
(Updated March 12, 2019, 4:30 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Addressed Ben's comment.
Repository: mesos
Description (updated)
-------
The method checks if the ResourceQuantities object
contains another ResourceQuantities.
Also added tests.
Diffs (updated)
-----
src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae
src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7
src/tests/resource_quantities_tests.cpp 435a4949b95e9a83be73781388eb4be9c7da695b
Diff: https://reviews.apache.org/r/70149/diff/2/
Changes: https://reviews.apache.org/r/70149/diff/1-2/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 70149: Added a `contains` method in
`ResourceQuantities`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70149/#review213603
-----------------------------------------------------------
src/common/resource_quantities.cpp
Lines 144-146 (patched)
<https://reviews.apache.org/r/70149/#comment299583>
Maybe for consistency use indexes here too?
Also, probably want to stick with 'this' and 'other' 'otherIndex' or 'left' and 'right' / 'rightIndex'?
src/common/resource_quantities.cpp
Lines 164 (patched)
<https://reviews.apache.org/r/70149/#comment299591>
Some comments on the conditions might help? (Ditto above in the loop)
// Right contains items not in left
src/tests/resource_quantities_tests.cpp
Lines 250-252 (patched)
<https://reviews.apache.org/r/70149/#comment299586>
do we need two? can we just use a single 'empty'?
src/tests/resource_quantities_tests.cpp
Lines 255-258 (patched)
<https://reviews.apache.org/r/70149/#comment299585>
maybe for readability s/these/some/ and s/those/empty/?
src/tests/resource_quantities_tests.cpp
Lines 261-263 (patched)
<https://reviews.apache.org/r/70149/#comment299587>
Do we need two different quantities here? Or can we just do:
```
x.contains(x)
```
src/tests/resource_quantities_tests.cpp
Lines 266-269 (patched)
<https://reviews.apache.org/r/70149/#comment299588>
how about 'superset' and 'subset'?
src/tests/resource_quantities_tests.cpp
Lines 274 (patched)
<https://reviews.apache.org/r/70149/#comment299589>
might as well check the other way around too?
src/tests/resource_quantities_tests.cpp
Lines 279 (patched)
<https://reviews.apache.org/r/70149/#comment299590>
Ditto here, might as well check the other way around too?
src/tests/resource_quantities_tests.cpp
Lines 282-283 (patched)
<https://reviews.apache.org/r/70149/#comment299592>
superset / subset again here for names?
- Benjamin Mahler
On March 7, 2019, 7:01 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70149/
> -----------------------------------------------------------
>
> (Updated March 7, 2019, 7:01 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The method checks if the `ResourceQuantities` object
> contains another `ResourceQuantities`.
>
> Also added tests.
>
>
> Diffs
> -----
>
> src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae
> src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7
> src/tests/resource_quantities_tests.cpp 435a4949b95e9a83be73781388eb4be9c7da695b
>
>
> Diff: https://reviews.apache.org/r/70149/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>