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/04 20:08:30 UTC
Re: Review Request 69599: Pulled up a new class `ResourceQuantities`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69599/
-----------------------------------------------------------
(Updated Jan. 4, 2019, 12:08 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Addressed Ben's comment.
Summary (updated)
-----------------
Pulled up a new class `ResourceQuantities`.
Repository: mesos
Description (updated)
-------
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/3/
Changes: https://reviews.apache.org/r/69599/diff/2-3/
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
>
>
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. 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 Meng Zhu <mz...@mesosphere.io>.
> On Jan. 4, 2019, 1:47 p.m., Benjamin Mahler wrote:
> > src/Makefile.am
> > Lines 1022-1023 (patched)
> > <https://reviews.apache.org/r/69599/diff/3/?file=2117686#file2117686line1022>
> >
> > Tabs are not aligned?
Fixed.
> On Jan. 4, 2019, 1:47 p.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.hpp
> > Lines 38 (patched)
> > <https://reviews.apache.org/r/69599/diff/3/?file=2117687#file2117687line38>
> >
> > The finite >=0 part isn't true anymore right?
Removed.
> On Jan. 4, 2019, 1:47 p.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.hpp
> > Lines 43 (patched)
> > <https://reviews.apache.org/r/69599/diff/3/?file=2117687#file2117687line43>
> >
> > "native" isn't clear here
Clarified
> On Jan. 4, 2019, 1:47 p.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.hpp
> > Lines 49-54 (patched)
> > <https://reviews.apache.org/r/69599/diff/3/?file=2117687#file2117687line49>
> >
> > "native" isn't clear here
> >
> > How about:
> >
> > ```
> > // An alternative design for this class was to provide addition and
> > // subtraction functions as opposed to a map interface. However, this
> > // leads to some un-obvious semantics around presence of zero values
> > // (will zero entries be automatically removed?) and handling of negatives
> > // (will negatives trigger a crash? will they be converted to zero? Note
> > // that Value::Scalar should be non-negative but there is currently no
> > // enforcement of this by Value::Scalar arithmetic operations; we probably
> > // want all Value::Scalar operations to go through the same negative
> > // handling). To avoid the confusion, we provided a map like interface
> > // which produces clear semantics: insertion works like maps, and the
> > // user is responsible for performing arithmetic operations on the values
> > // (using the already provided Value::Scalar arithmetic overloads).
> > ```
Done. Thanks.
> On Jan. 4, 2019, 1:47 p.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.hpp
> > Lines 83-88 (patched)
> > <https://reviews.apache.org/r/69599/diff/3/?file=2117687#file2117687line83>
> >
> > Put this in the .cpp?
Done.
> On Jan. 4, 2019, 1:47 p.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.hpp
> > Lines 103-117 (patched)
> > <https://reviews.apache.org/r/69599/diff/3/?file=2117687#file2117687line103>
> >
> > These are a bit verbose for the header, put them in the .cpp?
Done.
> On Jan. 4, 2019, 1:47 p.m., Benjamin Mahler wrote:
> > src/common/resource_quantities.cpp
> > Lines 39-69 (patched)
> > <https://reviews.apache.org/r/69599/diff/3/?file=2117688#file2117688line39>
> >
> > Let's make it more consistent with the Resources::fromSimpleString code and write down in a comment that it's what this is based on?
Done. Commented in the header.
> On Jan. 4, 2019, 1:47 p.m., Benjamin Mahler wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Lines 661-663 (original), 661-662 (patched)
> > <https://reviews.apache.org/r/69599/diff/3/?file=2117690#file2117690line661>
> >
> > Why the change to this line? Let's keep the diff small and relevant to this change only
Reverted.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69599/#review211690
-----------------------------------------------------------
On Jan. 5, 2019, 3: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, 3: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
>
>
Re: Review Request 69599: Pulled up a new class `ResourceQuantities`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69599/#review211690
-----------------------------------------------------------
Summary: "Pulled up a new class `ResourceQuantities`." -> "Pulled up the resource quantities class for more general use."
It's looking pretty good, just left some comments around parsing consistency, comments, and some minor things.
src/Makefile.am
Lines 1022-1023 (patched)
<https://reviews.apache.org/r/69599/#comment297160>
Tabs are not aligned?
src/common/resource_quantities.hpp
Lines 36-47 (patched)
<https://reviews.apache.org/r/69599/#comment297183>
Now that it's a map, I think this can be as simple as:
```
We provide map-like semantics: [] operator for inserting/retrieving values,
keys are unique, and iteration is sorted.
```
Along with the suggested paragraph below, that seems sufficient?
src/common/resource_quantities.hpp
Lines 38 (patched)
<https://reviews.apache.org/r/69599/#comment297178>
The finite >=0 part isn't true anymore right?
src/common/resource_quantities.hpp
Lines 43 (patched)
<https://reviews.apache.org/r/69599/#comment297182>
"native" isn't clear here
src/common/resource_quantities.hpp
Lines 44-46 (patched)
<https://reviews.apache.org/r/69599/#comment297179>
It looks like you can remove the note about inserting a new key if not present, that's consistent with map
src/common/resource_quantities.hpp
Lines 46-47 (patched)
<https://reviews.apache.org/r/69599/#comment297180>
mindful of producing negatives when mutating...
src/common/resource_quantities.hpp
Lines 49-54 (patched)
<https://reviews.apache.org/r/69599/#comment297181>
"native" isn't clear here
How about:
```
// An alternative design for this class was to provide addition and
// subtraction functions as opposed to a map interface. However, this
// leads to some un-obvious semantics around presence of zero values
// (will zero entries be automatically removed?) and handling of negatives
// (will negatives trigger a crash? will they be converted to zero? Note
// that Value::Scalar should be non-negative but there is currently no
// enforcement of this by Value::Scalar arithmetic operations; we probably
// want all Value::Scalar operations to go through the same negative
// handling). To avoid the confusion, we provided a map like interface
// which produces clear semantics: insertion works like maps, and the
// user is responsible for performing arithmetic operations on the values
// (using the already provided Value::Scalar arithmetic overloads).
```
src/common/resource_quantities.hpp
Lines 72-75 (patched)
<https://reviews.apache.org/r/69599/#comment297173>
Hm.. let's just make it consistent with Resources::fromSimpleString? I guess we can't just call it since we want to be more strict and validating post-call is messy.
But let's make the logic nearly the same and comment in the implementation of the function that it's what we based it off of?
src/common/resource_quantities.hpp
Lines 79-80 (patched)
<https://reviews.apache.org/r/69599/#comment297175>
otherwise an error is returned
src/common/resource_quantities.hpp
Lines 83-88 (patched)
<https://reviews.apache.org/r/69599/#comment297171>
Put this in the .cpp?
src/common/resource_quantities.hpp
Lines 96-117 (patched)
<https://reviews.apache.org/r/69599/#comment297184>
This is probably more obvious this way?
```
typedef std::vector<std::pair<std::string, Value::Scalar>>::const_iterator
iterator;
typedef std::vector<std::pair<std::string, Value::Scalar>>::const_iterator
const_iterator;
// NOTE: Non-`const` `begin()` and `end()` are __intentionally__
// defined with `const` semantics in order to prevent direct mutable access.
const_iterator begin();
const_iterator end();
```
But this comment isn't quite appropriate here, we only need the string name to be non-mutable now that it's a map interface. Can you update the comment accordingly? I'm guessing it's hard to actually provide a const string, non-const value iterator, so the comment is fine.
src/common/resource_quantities.hpp
Lines 103-117 (patched)
<https://reviews.apache.org/r/69599/#comment297172>
These are a bit verbose for the header, put them in the .cpp?
src/common/resource_quantities.cpp
Lines 39-69 (patched)
<https://reviews.apache.org/r/69599/#comment297177>
Let's make it more consistent with the Resources::fromSimpleString code and write down in a comment that it's what this is based on?
src/common/resource_quantities.cpp
Lines 51 (patched)
<https://reviews.apache.org/r/69599/#comment297176>
Let's re-use values::parse here to be consistent with how the resource parsing works and so that all value scalars are parsed the same way.
src/master/allocator/sorter/drf/sorter.hpp
Line 178 (original), 180 (patched)
<https://reviews.apache.org/r/69599/#comment297169>
Doesn't look like you would need mesos::internal here since we're already in that namespace, did you find you had to add it?
src/master/allocator/sorter/drf/sorter.hpp
Line 444 (original), 446 (patched)
<https://reviews.apache.org/r/69599/#comment297170>
Ditto here.
src/master/allocator/sorter/drf/sorter.cpp
Lines 661-663 (original), 661-662 (patched)
<https://reviews.apache.org/r/69599/#comment297163>
Why the change to this line? Let's keep the diff small and relevant to this change only
src/master/allocator/sorter/drf/sorter.cpp
Lines 670-676 (original), 669-677 (patched)
<https://reviews.apache.org/r/69599/#comment297164>
Not for this patch, but I'm just noticing this code seems easier to understand if s/scalar/total/
The if condition is a bit puzzling to read, maybe it would be clearer written as follows:
```
foreachpair (const string& resourceName,
const Value::Scalar& total,
total_.totals) {
if (total.value() <= 0.0) {
continue;
}
// Filter out the resources excluded from fair sharing.
if (fairnessExcludeResourceNames.isSome() &&
fairnessExcludeResourceNames->count(resourceName) > 0) {
continue;
}
Value::Scalar allocation = node->allocation.totals
.get(resourceName)
.getOrElse(Value::Scalar()); // Default to zero.
share = std::max(share, allocation / total.value());
}
```
src/master/allocator/sorter/drf/sorter.cpp
Lines 671-675 (original), 670-676 (patched)
<https://reviews.apache.org/r/69599/#comment297168>
The following seems simpler to read?
```
Value::Scalar allocation = node->allocation.totals
.get(resourceName)
.getOrElse(Value::Scalar()); // Default to zero.
share = std::max(share, allocation.value() / total.value());
```
src/master/allocator/sorter/random/sorter.hpp
Line 176 (original), 178 (patched)
<https://reviews.apache.org/r/69599/#comment297161>
Ditto here.
src/master/allocator/sorter/random/sorter.hpp
Line 420 (original), 422 (patched)
<https://reviews.apache.org/r/69599/#comment297162>
Ditto here.
- Benjamin Mahler
On Jan. 4, 2019, 8:08 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69599/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2019, 8:08 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/3/
>
>
> Testing
> -------
>
> make check
> Dedicated test in r/69600
>
>
> Thanks,
>
> Meng Zhu
>
>