mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Meng Zhu <m...@mesosphere.io>
Subject Re: Review Request 69599: Pulled up the resource quantities class for more general use.
Date Mon, 07 Jan 2019 17:22:18 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message