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