mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 69599: Added a new class `ResourceQuantities`.
Date Thu, 03 Jan 2019 00:03:05 GMT

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



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


src/common/quantities.hpp
Lines 33-38 (patched)
<https://reviews.apache.org/r/69599/#comment296985>

    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.



src/common/quantities.hpp
Lines 39 (patched)
<https://reviews.apache.org/r/69599/#comment296987>

    Let's just describe the model without the special NOTE sections? Here's a suggestion:
    
    ```
    // An efficient collection of resource quantities.
    //
    // E.g. [("cpus", 4.5), ("gpus", 0), ("ports", 1000)]
    //
    // With the following semantics:
    //
    //   (1) Each name is unique, and will have a finite value >= 0.
    //   (2) **Important**: Whenever a name is seen by this class
    //       (e.g. construction, subtract, add, etc), an entry will
    //       be added if non exists, and the entry will remain in
    //       the collection and cannot be removed. (This supports
    //       distinguishing explicit 0 quantities vs implicitly
    //       absent quantities).
    //   (3) `Value::Scalar` arguments must be non-negative and
    //       finite. Invalid arguments will result in an error where
    //       `Try` is returned and a CHECK failure otherwise.
    //   (4) If the result of subtraction is negative, the value
    //       will be normalized to 0 silently to maintain (1).
    //   (5) Iteration occurs on entries sorted by name.
    //
    // NOTE: For posterity, the status quo prior to this class
    // was to use  stripped-down `Resources` objects for storing
    // quantities, however this approach:
    //   
    //   (1) did not support quantities of non-scalar resources;
    //   (2) was error prone, the caller must ensure that no
    //       `Resource` metatdata (e.g. `DiskInfo`) is present;
    //   (3) had poor performance, given the `Resources` storage
    //       model and arithmetic implementation have to operate
    //       on broader invariants.
    //
    // TODO(mzhu): This class was based on an `ScalarResourceQuantities`
    // class added within the sorter to improve performance. It has been
    // duplicated here since we can't use its map-like interface; the
    // reason being that we want to normalize negative quantities to
    // zero after subtraction.
    class ResourceQuantities
    {
    
    };
    ```



src/common/quantities.hpp
Lines 42 (patched)
<https://reviews.apache.org/r/69599/#comment296988>

    comma separated?
    
    s/into `ResourceQuantities`//



src/common/quantities.hpp
Lines 43 (patched)
<https://reviews.apache.org/r/69599/#comment296989>

    Duplicate names are allowed in the input and will be merged into one entry.



src/common/quantities.hpp
Lines 45 (patched)
<https://reviews.apache.org/r/69599/#comment296990>

    This isn't valid code? Maybe just show the string?
    
    ```
    E.g. "cpus:10;mem:1024;ports:3"
    ```
    
    What happens if there's whitespace?
    
    ```
    "cpus:10 ; mem:1024 ; ports:3"
    ```



src/common/quantities.hpp
Lines 47 (patched)
<https://reviews.apache.org/r/69599/#comment296991>

    Should this be a TODO to consider supporting it? Seems like it might be useful to support
later?



src/common/quantities.hpp
Lines 49-50 (patched)
<https://reviews.apache.org/r/69599/#comment296992>

    This is a bit obvious and not very helpful to the reader, as a reader I would want to
know when it can return an error (e.g. bad format, negatives, NaN, ?whitespace?).



src/common/quantities.hpp
Lines 53 (patched)
<https://reviews.apache.org/r/69599/#comment297001>

    Let's add the optimization:
    
    ```
        // Pre-reserve space for first-class scalars.
        quantities.reserve(5u);  // [cpus, disk, gpus, mem, ports]
    ```



src/common/quantities.hpp
Lines 89-91 (patched)
<https://reviews.apache.org/r/69599/#comment296993>

    This seems more consistent?
    
    ```
    Option<Value::Scalar> get(const std::string& name) const;
    ```
    
    We're "getting" the entry for the name?



src/common/quantities.hpp
Lines 93-96 (patched)
<https://reviews.apache.org/r/69599/#comment296974>

    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?



src/common/quantities.hpp
Lines 95 (patched)
<https://reviews.apache.org/r/69599/#comment296973>

    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?



src/common/quantities.hpp
Lines 101 (patched)
<https://reviews.apache.org/r/69599/#comment297002>

    just call it `quantities` to make the code cleaner?



src/common/quantities.cpp
Lines 46 (patched)
<https://reviews.apache.org/r/69599/#comment296994>

    let's quote the token
    
    also, use colon for reason:
    
    ```
    ... quantities: missing ...
    ```



src/common/quantities.cpp
Lines 52 (patched)
<https://reviews.apache.org/r/69599/#comment296995>

    let's quote the input value
    
    also, `value: `



src/common/quantities.cpp
Lines 55 (patched)
<https://reviews.apache.org/r/69599/#comment296998>

    Might be a bit cleaner:
    ```
        if (*num < 0) {
    ```
    Ditto below



src/common/quantities.cpp
Lines 57 (patched)
<https://reviews.apache.org/r/69599/#comment296996>

    ditto here



src/common/quantities.cpp
Lines 58 (patched)
<https://reviews.apache.org/r/69599/#comment296999>

    quantity:



src/common/quantities.cpp
Lines 75-83 (patched)
<https://reviews.apache.org/r/69599/#comment297003>

    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();
    ```



src/common/quantities.cpp
Lines 94-110 (patched)
<https://reviews.apache.org/r/69599/#comment297004>

    ```
        // Find the entry or insert while maintaining
        // alphabetical ordering. Don't bother binary searching
        // since we don't expect a large number of quantities.
        auto it = quantities.begin();
        for (; it != quantities.end(); ++it) {
          if (it->first == name) {
            break;
          }
    
          if (it->first > name) {
            it = quantities.insert(
                it, std::make_pair(name, Value::Scalar()));
            break;
          }
        }
    
        it->second += scalar;
    ```



src/common/quantities.cpp
Lines 117-132 (patched)
<https://reviews.apache.org/r/69599/#comment297005>

    Ditto here, can you split the finding / inserting (loop) from the actual subtraction (after
the loop)?


- Benjamin Mahler


On Dec. 31, 2018, 5:52 a.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69599/
> -----------------------------------------------------------
> 
> (Updated Dec. 31, 2018, 5:52 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 adds a dedicated class `ResourceQuantities` for this
> purpose. Underneath, it contains a vector of name and quantity
> pairs. All quantities with the same name are merged.
> 
> This patch also adds a wrapper class `Quantity` to abstract the
> quantity concept. While currently it only holds a `Value::Scalar`
> underneath, in the future, we might want to modify it to support
> fixed point representation or units.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8 
>   src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa 
>   src/common/quantities.hpp PRE-CREATION 
>   src/common/quantities.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69599/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> Dedicated test in r/69600
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


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