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 a new class `ResourceQuantities`.
Date Fri, 04 Jan 2019 20:09:29 GMT


> On Jan. 2, 2019, 4:03 p.m., Benjamin Mahler wrote:
> > In the commit description can you add context about the other class we already introduced,
along with why we are adding this sister class with a non-map like interface? (Looks like
it's because we want the extra Value::Scalar validation that isn't built in to Value::Scalar
arithmetic operators, so we have to take Value::Scalar in the interface rather than exposing
them for the caller to manipulate?)

As discussed offline, let's preserve the map interface and thus it is very similar to `ScalarResourceQuantities`,
so I will just "pull it up".


> On Jan. 2, 2019, 4:03 p.m., Benjamin Mahler wrote:
> > src/common/quantities.hpp
> > Lines 33-38 (patched)
> > <https://reviews.apache.org/r/69599/diff/2/?file=2116924#file2116924line33>
> >
> >     Can you also add some context about the class we already added here and why
we duplicated it? Along with the plan to remove it in favor of this interface? This can all
be in a TODO.

As mentioned above, this patch will just pull that class up.


> On Jan. 2, 2019, 4:03 p.m., Benjamin Mahler wrote:
> > src/common/quantities.hpp
> > Lines 47 (patched)
> > <https://reviews.apache.org/r/69599/diff/2/?file=2116924#file2116924line47>
> >
> >     Should this be a TODO to consider supporting it? Seems like it might be useful
to support later?

Done.


> On Jan. 2, 2019, 4:03 p.m., Benjamin Mahler wrote:
> > src/common/quantities.hpp
> > Lines 53 (patched)
> > <https://reviews.apache.org/r/69599/diff/2/?file=2116924#file2116924line53>
> >
> >     Let's add the optimization:
> >     
> >     ```
> >         // Pre-reserve space for first-class scalars.
> >         quantities.reserve(5u);  // [cpus, disk, gpus, mem, ports]
> >     ```

Done.


> On Jan. 2, 2019, 4:03 p.m., Benjamin Mahler wrote:
> > src/common/quantities.hpp
> > Lines 89-91 (patched)
> > <https://reviews.apache.org/r/69599/diff/2/?file=2116924#file2116924line89>
> >
> >     This seems more consistent?
> >     
> >     ```
> >     Option<Value::Scalar> get(const std::string& name) const;
> >     ```
> >     
> >     We're "getting" the entry for the name?

Done.


> On Jan. 2, 2019, 4:03 p.m., Benjamin Mahler wrote:
> > src/common/quantities.hpp
> > Lines 93-96 (patched)
> > <https://reviews.apache.org/r/69599/diff/2/?file=2116924#file2116924line93>
> >
> >     Shouldn't we CHECK that the incoming scalars here are non-negative and `isfinite()`
rather than silently ignoring those cases?
> >     
> >     Negative scalars as the input here are not ignored by the implementation?

As discussed offline, we will provide map interface and allow direct mutation in favor of
providing arithmetic methods.


> On Jan. 2, 2019, 4:03 p.m., Benjamin Mahler wrote:
> > src/common/quantities.hpp
> > Lines 95 (patched)
> > <https://reviews.apache.org/r/69599/diff/2/?file=2116924#file2116924line95>
> >
> >     This is a bit confusing, it seems like the reader should be reading / understanding
the model from the top of the class rather than here. (FWIW, add also has the same implications
on the model, if there's no entry, and you `add()` a zero, will there be a zero stored? what
if you `add()` a negative?

See above.


> On Jan. 2, 2019, 4:03 p.m., Benjamin Mahler wrote:
> > src/common/quantities.hpp
> > Lines 101 (patched)
> > <https://reviews.apache.org/r/69599/diff/2/?file=2116924#file2116924line101>
> >
> >     just call it `quantities` to make the code cleaner?

Done.


> On Jan. 2, 2019, 4:03 p.m., Benjamin Mahler wrote:
> > src/common/quantities.cpp
> > Lines 46 (patched)
> > <https://reviews.apache.org/r/69599/diff/2/?file=2116925#file2116925line46>
> >
> >     let's quote the token
> >     
> >     also, use colon for reason:
> >     
> >     ```
> >     ... quantities: missing ...
> >     ```

Done.


> On Jan. 2, 2019, 4:03 p.m., Benjamin Mahler wrote:
> > src/common/quantities.cpp
> > Lines 55 (patched)
> > <https://reviews.apache.org/r/69599/diff/2/?file=2116925#file2116925line55>
> >
> >     Might be a bit cleaner:
> >     ```
> >         if (*num < 0) {
> >     ```
> >     Ditto below

We do not have `*` operator for `Try`?


> On Jan. 2, 2019, 4:03 p.m., Benjamin Mahler wrote:
> > src/common/quantities.cpp
> > Lines 75-83 (patched)
> > <https://reviews.apache.org/r/69599/diff/2/?file=2116925#file2116925line75>
> >
> >     Why does this need iterators? Can't this just be a foreach with a break?
> >     
> >     ```
> >         foreach (auto& quantity, quantities) {
> >           if (quantity.first == name) {
> >             return quantity.second;
> >           } else if (it->first > name) {
> >             break;
> >           }
> >         }
> >         
> >         return None();
> >     ```

Done.


- Meng


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69599/#review211620
-----------------------------------------------------------


On Jan. 4, 2019, 12: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, 12: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
> 
>


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