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