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 Sat, 05 Jan 2019 23:33:19 GMT


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


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