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/01/05 23:28:57 UTC
Re: Review Request 69599: Pulled up the resource quantities class for
more general use.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69599/
-----------------------------------------------------------
(Updated Jan. 5, 2019, 3:28 p.m.)
Review request for mesos and Benjamin Mahler.
Summary (updated)
-----------------
Pulled up the resource quantities class for more general use.
Repository: mesos
Description
-------
There are many places that we need to express the concept of
resource quantities such as enforcing allocation quantities
in the allocator and set guaranteed resource quantities with quota.
However, the current code usually uses the complex Resources
class which is unnecessary and inefficient.
This patch pulls class ScalarResourceQuantities in sorter.hpp
up, aiming to use it for all resource quantity related usages.
We mostly preserve the map interface and added other utilities such
as parsing.
Diffs (updated)
-----
src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8
src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa
src/common/resource_quantities.hpp PRE-CREATION
src/common/resource_quantities.cpp PRE-CREATION
src/master/allocator/sorter/drf/sorter.hpp 084df82baef91eca5775b0bd17d943f3fb8df70b
src/master/allocator/sorter/drf/sorter.cpp a648fac7e922ab0aefdf9363624d7242f1fc6588
src/master/allocator/sorter/random/sorter.hpp 800b22c67126c2b14c5259d9d122d2e196cc80d8
src/master/allocator/sorter/sorter.hpp 68cf6988ef1a156cf16951e3164261234e9abeeb
Diff: https://reviews.apache.org/r/69599/diff/4/
Changes: https://reviews.apache.org/r/69599/diff/3-4/
Testing
-------
make check
Dedicated test in r/69600
Thanks,
Meng Zhu
Re: Review Request 69599: Pulled up the resource quantities class for
more general use.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69599/#review211727
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Mahler
On Jan. 7, 2019, 6:23 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69599/
> -----------------------------------------------------------
>
> (Updated Jan. 7, 2019, 6:23 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> There are many places that we need to express the concept of
> resource quantities such as enforcing allocation quantities
> in the allocator and set guaranteed resource quantities with quota.
> However, the current code usually uses the complex Resources
> class which is unnecessary and inefficient.
>
> This patch pulls class ScalarResourceQuantities in sorter.hpp
> up, aiming to use it for all resource quantity related usages.
> We mostly preserve the map interface and added other utilities such
> as parsing.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8
> src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa
> src/common/resource_quantities.hpp PRE-CREATION
> src/common/resource_quantities.cpp PRE-CREATION
> src/master/allocator/sorter/drf/sorter.hpp 084df82baef91eca5775b0bd17d943f3fb8df70b
> src/master/allocator/sorter/drf/sorter.cpp a648fac7e922ab0aefdf9363624d7242f1fc6588
> src/master/allocator/sorter/random/sorter.hpp 800b22c67126c2b14c5259d9d122d2e196cc80d8
> src/master/allocator/sorter/sorter.hpp 68cf6988ef1a156cf16951e3164261234e9abeeb
>
>
> Diff: https://reviews.apache.org/r/69599/diff/5/
>
>
> Testing
> -------
>
> make check
> Dedicated test in r/69600
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69599: Pulled up the resource quantities class for
more general use.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69599/
-----------------------------------------------------------
(Updated Jan. 6, 2019, 10:23 p.m.)
Review request for mesos and Benjamin Mahler.
Repository: mesos
Description
-------
There are many places that we need to express the concept of
resource quantities such as enforcing allocation quantities
in the allocator and set guaranteed resource quantities with quota.
However, the current code usually uses the complex Resources
class which is unnecessary and inefficient.
This patch pulls class ScalarResourceQuantities in sorter.hpp
up, aiming to use it for all resource quantity related usages.
We mostly preserve the map interface and added other utilities such
as parsing.
Diffs (updated)
-----
src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8
src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa
src/common/resource_quantities.hpp PRE-CREATION
src/common/resource_quantities.cpp PRE-CREATION
src/master/allocator/sorter/drf/sorter.hpp 084df82baef91eca5775b0bd17d943f3fb8df70b
src/master/allocator/sorter/drf/sorter.cpp a648fac7e922ab0aefdf9363624d7242f1fc6588
src/master/allocator/sorter/random/sorter.hpp 800b22c67126c2b14c5259d9d122d2e196cc80d8
src/master/allocator/sorter/sorter.hpp 68cf6988ef1a156cf16951e3164261234e9abeeb
Diff: https://reviews.apache.org/r/69599/diff/5/
Changes: https://reviews.apache.org/r/69599/diff/4-5/
Testing
-------
make check
Dedicated test in r/69600
Thanks,
Meng Zhu
Re: Review Request 69599: Pulled up the resource quantities class for
more general use.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On Jan. 6, 2019, 9:14 p.m., Benjamin Mahler wrote:
> >
oops, forgot to publish the comment.
> On Jan. 6, 2019, 9:14 p.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.hpp
> > Lines 81 (patched)
> > <https://reviews.apache.org/r/69599/diff/4/?file=2117853#file2117853line81>
> >
> > This TODO looks like it belongs elsewhere, like at the top or after this function, or just remove it?
Removed.
> On Jan. 6, 2019, 9:14 p.m., Benjamin Mahler wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Line 671 (original), 671 (patched)
> > <https://reviews.apache.org/r/69599/diff/4/?file=2117856#file2117856line671>
> >
> > Why do you say to consider this, we actually don't do anything to ensure this AFAIK, so it would fail, no?
This denotes total resources in the cluster, conceptually should never be negative? I glanced the code, this invariant seems to hold (make check also passes). But I removed it. Let's revisit it later.
> On Jan. 6, 2019, 9:14 p.m., Benjamin Mahler wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Lines 675-676 (original), 678-681 (patched)
> > <https://reviews.apache.org/r/69599/diff/4/?file=2117856#file2117856line678>
> >
> > I find the if condition here adds unnecessary load on the reader, there isn't really anything special about 0 to warrant a special case?
> >
> > E.g.
> >
> > ```
> > Value::Scalar allocation = node->allocation.totals
> > .get(resourceName)
> > .getOrElse(Value::Scalar()); // Absent means zero.
> >
> > share = std::max(share, allocation.value() / total.value());
> > ```
> >
> > If this is a performance optimization, can you show it's helpful?
> >
> > It looks like this comment was missed in the earlier review?
Sorry, I forgot to reply to the comment earlier. Yeah, I was mainly thinking of performance since this is a performance critical place. Took your suggestion, will do a profiling soon.
> On Jan. 6, 2019, 9:14 p.m., Benjamin Mahler wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Lines 680 (patched)
> > <https://reviews.apache.org/r/69599/diff/4/?file=2117856#file2117856line680>
> >
> > As a general rule I would suggest using CHECK_NOTNONE in cases where we do unguarded retrieval, e.g.
> >
> > ```
> > CHECK_NOTNONE(Resources::parse("cpus:1"))
> > ```
> >
> > In cases where the logic around whether it's some is complex and we want to sanity check it, probably CHECK_SOME is better:
> >
> > ```
> > // Many cases but should always lead to some
> >
> > CHECK_SOME(v);
> >
> > some code
> > v->foo();
> > ```
> >
> > But in cases like this, where we're actually inside the guarded block, just use ->
> >
> > ```
> > if (allocation.isSome()) {
> > share = std::max(share, allocation->value() / scalar.value());
> > ```
Thanks! Will keep this in mind.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69599/#review211714
-----------------------------------------------------------
On Jan. 6, 2019, 10:23 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69599/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2019, 10:23 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> There are many places that we need to express the concept of
> resource quantities such as enforcing allocation quantities
> in the allocator and set guaranteed resource quantities with quota.
> However, the current code usually uses the complex Resources
> class which is unnecessary and inefficient.
>
> This patch pulls class ScalarResourceQuantities in sorter.hpp
> up, aiming to use it for all resource quantity related usages.
> We mostly preserve the map interface and added other utilities such
> as parsing.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8
> src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa
> src/common/resource_quantities.hpp PRE-CREATION
> src/common/resource_quantities.cpp PRE-CREATION
> src/master/allocator/sorter/drf/sorter.hpp 084df82baef91eca5775b0bd17d943f3fb8df70b
> src/master/allocator/sorter/drf/sorter.cpp a648fac7e922ab0aefdf9363624d7242f1fc6588
> src/master/allocator/sorter/random/sorter.hpp 800b22c67126c2b14c5259d9d122d2e196cc80d8
> src/master/allocator/sorter/sorter.hpp 68cf6988ef1a156cf16951e3164261234e9abeeb
>
>
> Diff: https://reviews.apache.org/r/69599/diff/5/
>
>
> Testing
> -------
>
> make check
> Dedicated test in r/69600
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69599: Pulled up the resource quantities class for
more general use.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Jan. 7, 2019, 5:14 a.m., Benjamin Mahler wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Line 671 (original), 671 (patched)
> > <https://reviews.apache.org/r/69599/diff/4/?file=2117856#file2117856line671>
> >
> > Why do you say to consider this, we actually don't do anything to ensure this AFAIK, so it would fail, no?
>
> Meng Zhu wrote:
> This denotes total resources in the cluster, conceptually should never be negative? I glanced the code, this invariant seems to hold (make check also passes). But I removed it. Let's revisit it later.
> conceptually should never be negative?
Whoops, I initially read the check to be > 0 (and that's what my comment was referring to: 0 seems to be possible).
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69599/#review211714
-----------------------------------------------------------
On Jan. 7, 2019, 6:23 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69599/
> -----------------------------------------------------------
>
> (Updated Jan. 7, 2019, 6:23 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> There are many places that we need to express the concept of
> resource quantities such as enforcing allocation quantities
> in the allocator and set guaranteed resource quantities with quota.
> However, the current code usually uses the complex Resources
> class which is unnecessary and inefficient.
>
> This patch pulls class ScalarResourceQuantities in sorter.hpp
> up, aiming to use it for all resource quantity related usages.
> We mostly preserve the map interface and added other utilities such
> as parsing.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8
> src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa
> src/common/resource_quantities.hpp PRE-CREATION
> src/common/resource_quantities.cpp PRE-CREATION
> src/master/allocator/sorter/drf/sorter.hpp 084df82baef91eca5775b0bd17d943f3fb8df70b
> src/master/allocator/sorter/drf/sorter.cpp a648fac7e922ab0aefdf9363624d7242f1fc6588
> src/master/allocator/sorter/random/sorter.hpp 800b22c67126c2b14c5259d9d122d2e196cc80d8
> src/master/allocator/sorter/sorter.hpp 68cf6988ef1a156cf16951e3164261234e9abeeb
>
>
> Diff: https://reviews.apache.org/r/69599/diff/5/
>
>
> Testing
> -------
>
> make check
> Dedicated test in r/69600
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69599: Pulled up the resource quantities class for
more general use.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69599/#review211714
-----------------------------------------------------------
src/common/resource_quantities.hpp
Lines 81 (patched)
<https://reviews.apache.org/r/69599/#comment297241>
This TODO looks like it belongs elsewhere, like at the top or after this function, or just remove it?
src/common/resource_quantities.cpp
Lines 37 (patched)
<https://reviews.apache.org/r/69599/#comment297242>
Would be nice to have a comment here that this is based on fromSimpleString so that anyone reading it and wondering why it's done this way knows, as well as if they try to change the implementation they'll think about that. Right now it's only mentioned in the header which is unlikely to be spotted when looking at the implementation.
src/common/resource_quantities.cpp
Lines 45 (patched)
<https://reviews.apache.org/r/69599/#comment297243>
Why not just stick with "Failed to parse " like the other messages?
```
"Failed to parse '" + token + "'"
": expected 1 ':' but found " + stringify(pair.size());
```
src/common/resource_quantities.cpp
Lines 56-63 (patched)
<https://reviews.apache.org/r/69599/#comment297245>
Can you close the quote on the same line for these?
Also, seems more readable for these if the second line is reason only?
```
"Failed to parse '" + pair[1] + "' to quantity:"
" only scalar values are allowed");
```
src/master/allocator/sorter/drf/sorter.cpp
Line 671 (original), 671 (patched)
<https://reviews.apache.org/r/69599/#comment297248>
Why do you say to consider this, we actually don't do anything to ensure this AFAIK, so it would fail, no?
src/master/allocator/sorter/drf/sorter.cpp
Lines 675-676 (original), 678-681 (patched)
<https://reviews.apache.org/r/69599/#comment297246>
I find the if condition here adds unnecessary load on the reader, there isn't really anything special about 0 to warrant a special case?
E.g.
```
Value::Scalar allocation = node->allocation.totals
.get(resourceName)
.getOrElse(Value::Scalar()); // Absent means zero.
share = std::max(share, allocation.value() / total.value());
```
If this is a performance optimization, can you show it's helpful?
It looks like this comment was missed in the earlier review?
src/master/allocator/sorter/drf/sorter.cpp
Lines 680 (patched)
<https://reviews.apache.org/r/69599/#comment297247>
As a general rule I would suggest using CHECK_NOTNONE in cases where we do unguarded retrieval, e.g.
```
CHECK_NOTNONE(Resources::parse("cpus:1"))
```
In cases where the logic around whether it's some is complex and we want to sanity check it, probably CHECK_SOME is better:
```
// Many cases but should always lead to some
CHECK_SOME(v);
some code
v->foo();
```
But in cases like this, where we're actually inside the guarded block, just use ->
```
if (allocation.isSome()) {
share = std::max(share, allocation->value() / scalar.value());
```
- Benjamin Mahler
On Jan. 5, 2019, 11:28 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69599/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2019, 11:28 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> There are many places that we need to express the concept of
> resource quantities such as enforcing allocation quantities
> in the allocator and set guaranteed resource quantities with quota.
> However, the current code usually uses the complex Resources
> class which is unnecessary and inefficient.
>
> This patch pulls class ScalarResourceQuantities in sorter.hpp
> up, aiming to use it for all resource quantity related usages.
> We mostly preserve the map interface and added other utilities such
> as parsing.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8
> src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa
> src/common/resource_quantities.hpp PRE-CREATION
> src/common/resource_quantities.cpp PRE-CREATION
> src/master/allocator/sorter/drf/sorter.hpp 084df82baef91eca5775b0bd17d943f3fb8df70b
> src/master/allocator/sorter/drf/sorter.cpp a648fac7e922ab0aefdf9363624d7242f1fc6588
> src/master/allocator/sorter/random/sorter.hpp 800b22c67126c2b14c5259d9d122d2e196cc80d8
> src/master/allocator/sorter/sorter.hpp 68cf6988ef1a156cf16951e3164261234e9abeeb
>
>
> Diff: https://reviews.apache.org/r/69599/diff/4/
>
>
> Testing
> -------
>
> make check
> Dedicated test in r/69600
>
>
> Thanks,
>
> Meng Zhu
>
>