> On Feb. 27, 2019, 9:04 a.m., Benjamin Mahler wrote: > > src/common/resource_quantities.cpp > > Lines 61-64 (original), 61-64 (patched) > > <https://reviews.apache.org/r/70062/diff/1/?file=2127049#file2127049line61> > > > > Ditto comment below, it seems 0 should be allowed here.
Agreed. Done. > On Feb. 27, 2019, 9:04 a.m., Benjamin Mahler wrote: > > src/common/resource_quantities.cpp > > Lines 141-178 (patched) > > <https://reviews.apache.org/r/70062/diff/1/?file=2127049#file2127049line141> > > > > Can you make these more symmetrical? > > > > If we want to keep the [] operator, why don't we use it in -= and also > > use a new .erase function? > > > > ``` > > // TODO: Walk both vectors at the same time, rather than re-searching. > > foreach (auto& that, those) { > > (*this)[that.first] -= that.second; > > > > if ((*this)[that.first] == 0) { > > erase(that.first); > > } > > } > > > > return *this; > > ``` > > > > There doesn't seem to be a big benefit from the existing code in this > > patch for the complexity it has? It still is re-searching the list from the > > beginning each iteration as opposed to walking both lists assuming > > alphabetical ordering for better performance. Made them symmetrical, both take advantage of the ordering and are now one-pass. If we just do the loop thing, it kinds of defeat the purpose of ordering. Anyway, these are going to be used in performance critical places, so a bit of performance gain for the complexity seems to be worth it. `[]` is still kept, mainly for the (current and future) constructors and conversion methods. > On Feb. 27, 2019, 9:04 a.m., Benjamin Mahler wrote: > > src/master/allocator/sorter/drf/sorter.hpp > > Line 347 (original), 347 (patched) > > <https://reviews.apache.org/r/70062/diff/1/?file=2127050#file2127050line347> > > > > Seems like we sould just allow an implicit constructor to make this > > seamless? > > > > ``` > > totals += quantitiesToAdd; > > ``` I want to distinguish this from future conversations interfaces where non-scalar resources are also converted into quantities. > On Feb. 27, 2019, 9:04 a.m., Benjamin Mahler wrote: > > src/tests/resource_quantities_tests.cpp > > Lines 116-118 (patched) > > <https://reviews.apache.org/r/70062/diff/1/?file=2127054#file2127054line123> > > > > Is this consistent with Resources? This seems fine and it should just > > be not stored. Yep, fixed. - Meng ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70062/#review213260 ----------------------------------------------------------- On Feb. 26, 2019, 5:29 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70062/ > ----------------------------------------------------------- > > (Updated Feb. 26, 2019, 5:29 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-9608 > https://issues.apache.org/jira/browse/MESOS-9608 > > > Repository: mesos > > > Description > ------- > > This patch removed the map interface of > `class ResourceQuantities`, added a few built-in > arithmetic operations. Now, absent resource items imply > there is no (zero) such resources. > > Also added a to-do to add `class ResourceLimits` which > is similar but treats absent resource entries as having > infinite amount of such resource. > > Also changed affected call sites and tests accordingly. > > > Diffs > ----- > > src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae > src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 > src/master/allocator/sorter/drf/sorter.hpp > e64c9ad3520a601f7854e807ef5306d5bffc0ff8 > src/master/allocator/sorter/drf/sorter.cpp > b128df08e3c93d3d1a75c637cbed359c2cb8cda4 > src/master/allocator/sorter/random/sorter.hpp > 4f230ec740e2f80d5333c61c5b23d9a631bdb273 > src/master/allocator/sorter/random/sorter.cpp > f578ef19b4dee9cf9c7c99a8988829ecde70ed6d > src/tests/resource_quantities_tests.cpp > 435a4949b95e9a83be73781388eb4be9c7da695b > > > Diff: https://reviews.apache.org/r/70062/diff/2/ > > > Testing > ------- > > make check > Dedicated tests are added in the subsequent patch. > > > Thanks, > > Meng Zhu > >
