You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2019/01/03 17:14:36 UTC
Re: Review Request 69601: Added a `Resources` method
`contains(ResourceQuantities)`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69601/#review211638
-----------------------------------------------------------
include/mesos/resources.hpp
Lines 45 (patched)
<https://reviews.apache.org/r/69601/#comment297027>
This is a public header and I don't think the internal headers get installed so this would break any use of the public headers?
Can you forward declare instead?
src/common/resources.cpp
Lines 1517 (patched)
<https://reviews.apache.org/r/69601/#comment297028>
Do we need the iterator? Can we use a foreach auto loop here?
src/common/resources.cpp
Lines 1518 (patched)
<https://reviews.apache.org/r/69601/#comment297030>
s/remainingAmount/remaining/
src/common/resources.cpp
Lines 1523-1525 (patched)
<https://reviews.apache.org/r/69601/#comment297031>
Maybe we can make this a bit more tidy?
```
case Value::SCALAR: remaining -= r.scalar().value(); break;
case Value::SET: remaining -= r.set().item_size(); break;
case ...
```
src/common/resources.cpp
Lines 1529 (patched)
<https://reviews.apache.org/r/69601/#comment297029>
Why the comment here but not below on the same optimization?
It seems fine without the comment in both places to me
src/tests/resources_tests.cpp
Lines 2054 (patched)
<https://reviews.apache.org/r/69601/#comment297032>
Maybe we can make the tests more readable with some better variable names or lambdas that allow us to see the entire comparision on one line?
E.g.
```
EXPECT_TRUE(Resources().contains(ResourceQuantities());
EXPECT_FALSE(nonEmptyResources.contains(emptyQuantities);
auto resources = [](const string& s) { ... };
auto quantities = [](const string& s) { ... };
// Can easily read what's being checked:
EXPECT_TRUE(resources("cpus:1;mem:2")
.contains(quantities("cpus:0.1;mem:1"));
```
- Benjamin Mahler
On Dec. 31, 2018, 5:52 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69601/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2018, 5:52 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This method checks if the quantities of this `Resources` is a
> superset of the given `ResourceQuantities`. If a `Resource` object
> is `SCALAR` type, its quantity is its scalar value. For `RANGES`
> and `SET` type, their quantities are the number of different
> instances in the range or set. For example, "range:[1-5]" has a
> quantity of 5 and "set:{a,b}" has a quantity of 2.
>
> Also added a dedicated test.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 36ccf0e1135ce15898d31db51c80fa7b5826907b
> include/mesos/v1/resources.hpp 1a9ea44e78d985286e04e040879d022838ddcc3f
> src/common/resources.cpp 758b5a2fac101bfb0a07ba76c204b02d1554b3ac
> src/tests/resources_tests.cpp 5337a5c9af9431cc1c7822c78945ffc369488900
> src/v1/resources.cpp d717f5327a45cfe9bc075bc3acf37316223d389d
>
>
> Diff: https://reviews.apache.org/r/69601/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69601: Added a `Resources` method
`contains(ResourceQuantities)`.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On Jan. 3, 2019, 9:14 a.m., Benjamin Mahler wrote:
> > include/mesos/resources.hpp
> > Lines 45 (patched)
> > <https://reviews.apache.org/r/69601/diff/2/?file=2116929#file2116929line45>
> >
> > This is a public header and I don't think the internal headers get installed so this would break any use of the public headers?
> >
> > Can you forward declare instead?
Done.
> On Jan. 3, 2019, 9:14 a.m., Benjamin Mahler wrote:
> > src/common/resources.cpp
> > Lines 1517 (patched)
> > <https://reviews.apache.org/r/69601/diff/2/?file=2116931#file2116931line1517>
> >
> > Do we need the iterator? Can we use a foreach auto loop here?
Done.
> On Jan. 3, 2019, 9:14 a.m., Benjamin Mahler wrote:
> > src/common/resources.cpp
> > Lines 1523-1525 (patched)
> > <https://reviews.apache.org/r/69601/diff/2/?file=2116931#file2116931line1523>
> >
> > Maybe we can make this a bit more tidy?
> >
> > ```
> > case Value::SCALAR: remaining -= r.scalar().value(); break;
> > case Value::SET: remaining -= r.set().item_size(); break;
> > case ...
> > ```
Done.
> On Jan. 3, 2019, 9:14 a.m., Benjamin Mahler wrote:
> > src/common/resources.cpp
> > Lines 1529 (patched)
> > <https://reviews.apache.org/r/69601/diff/2/?file=2116931#file2116931line1529>
> >
> > Why the comment here but not below on the same optimization?
> >
> > It seems fine without the comment in both places to me
Done.
> On Jan. 3, 2019, 9:14 a.m., Benjamin Mahler wrote:
> > src/tests/resources_tests.cpp
> > Lines 2054 (patched)
> > <https://reviews.apache.org/r/69601/diff/2/?file=2116932#file2116932line2054>
> >
> > Maybe we can make the tests more readable with some better variable names or lambdas that allow us to see the entire comparision on one line?
> >
> > E.g.
> >
> > ```
> > EXPECT_TRUE(Resources().contains(ResourceQuantities());
> >
> > EXPECT_FALSE(nonEmptyResources.contains(emptyQuantities);
> >
> > auto resources = [](const string& s) { ... };
> > auto quantities = [](const string& s) { ... };
> >
> > // Can easily read what's being checked:
> > EXPECT_TRUE(resources("cpus:1;mem:2")
> > .contains(quantities("cpus:0.1;mem:1"));
> > ```
Done, much cleaner, thanks!
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69601/#review211638
-----------------------------------------------------------
On Jan. 4, 2019, 12:16 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69601/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2019, 12:16 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This method checks if the quantities of this `Resources` is a
> superset of the given `ResourceQuantities`. If a `Resource` object
> is `SCALAR` type, its quantity is its scalar value. For `RANGES`
> and `SET` type, their quantities are the number of different
> instances in the range or set. For example, "range:[1-5]" has a
> quantity of 5 and "set:{a,b}" has a quantity of 2.
>
> Also added a dedicated test.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 36ccf0e1135ce15898d31db51c80fa7b5826907b
> include/mesos/v1/resources.hpp 1a9ea44e78d985286e04e040879d022838ddcc3f
> src/common/resources.cpp 758b5a2fac101bfb0a07ba76c204b02d1554b3ac
> src/tests/resources_tests.cpp 5337a5c9af9431cc1c7822c78945ffc369488900
> src/v1/resources.cpp d717f5327a45cfe9bc075bc3acf37316223d389d
>
>
> Diff: https://reviews.apache.org/r/69601/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>