----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69599/#review211690 -----------------------------------------------------------
Summary: "Pulled up a new class `ResourceQuantities`." -> "Pulled up the resource quantities class for more general use." It's looking pretty good, just left some comments around parsing consistency, comments, and some minor things. src/Makefile.am Lines 1022-1023 (patched) <https://reviews.apache.org/r/69599/#comment297160> Tabs are not aligned? src/common/resource_quantities.hpp Lines 36-47 (patched) <https://reviews.apache.org/r/69599/#comment297183> Now that it's a map, I think this can be as simple as: ``` We provide map-like semantics: [] operator for inserting/retrieving values, keys are unique, and iteration is sorted. ``` Along with the suggested paragraph below, that seems sufficient? src/common/resource_quantities.hpp Lines 38 (patched) <https://reviews.apache.org/r/69599/#comment297178> The finite >=0 part isn't true anymore right? src/common/resource_quantities.hpp Lines 43 (patched) <https://reviews.apache.org/r/69599/#comment297182> "native" isn't clear here src/common/resource_quantities.hpp Lines 44-46 (patched) <https://reviews.apache.org/r/69599/#comment297179> It looks like you can remove the note about inserting a new key if not present, that's consistent with map src/common/resource_quantities.hpp Lines 46-47 (patched) <https://reviews.apache.org/r/69599/#comment297180> mindful of producing negatives when mutating... src/common/resource_quantities.hpp Lines 49-54 (patched) <https://reviews.apache.org/r/69599/#comment297181> "native" isn't clear here How about: ``` // An alternative design for this class was to provide addition and // subtraction functions as opposed to a map interface. However, this // leads to some un-obvious semantics around presence of zero values // (will zero entries be automatically removed?) and handling of negatives // (will negatives trigger a crash? will they be converted to zero? Note // that Value::Scalar should be non-negative but there is currently no // enforcement of this by Value::Scalar arithmetic operations; we probably // want all Value::Scalar operations to go through the same negative // handling). To avoid the confusion, we provided a map like interface // which produces clear semantics: insertion works like maps, and the // user is responsible for performing arithmetic operations on the values // (using the already provided Value::Scalar arithmetic overloads). ``` src/common/resource_quantities.hpp Lines 72-75 (patched) <https://reviews.apache.org/r/69599/#comment297173> Hm.. let's just make it consistent with Resources::fromSimpleString? I guess we can't just call it since we want to be more strict and validating post-call is messy. But let's make the logic nearly the same and comment in the implementation of the function that it's what we based it off of? src/common/resource_quantities.hpp Lines 79-80 (patched) <https://reviews.apache.org/r/69599/#comment297175> otherwise an error is returned src/common/resource_quantities.hpp Lines 83-88 (patched) <https://reviews.apache.org/r/69599/#comment297171> Put this in the .cpp? src/common/resource_quantities.hpp Lines 96-117 (patched) <https://reviews.apache.org/r/69599/#comment297184> This is probably more obvious this way? ``` typedef std::vector<std::pair<std::string, Value::Scalar>>::const_iterator iterator; typedef std::vector<std::pair<std::string, Value::Scalar>>::const_iterator const_iterator; // NOTE: Non-`const` `begin()` and `end()` are __intentionally__ // defined with `const` semantics in order to prevent direct mutable access. const_iterator begin(); const_iterator end(); ``` But this comment isn't quite appropriate here, we only need the string name to be non-mutable now that it's a map interface. Can you update the comment accordingly? I'm guessing it's hard to actually provide a const string, non-const value iterator, so the comment is fine. src/common/resource_quantities.hpp Lines 103-117 (patched) <https://reviews.apache.org/r/69599/#comment297172> These are a bit verbose for the header, put them in the .cpp? src/common/resource_quantities.cpp Lines 39-69 (patched) <https://reviews.apache.org/r/69599/#comment297177> Let's make it more consistent with the Resources::fromSimpleString code and write down in a comment that it's what this is based on? src/common/resource_quantities.cpp Lines 51 (patched) <https://reviews.apache.org/r/69599/#comment297176> Let's re-use values::parse here to be consistent with how the resource parsing works and so that all value scalars are parsed the same way. src/master/allocator/sorter/drf/sorter.hpp Line 178 (original), 180 (patched) <https://reviews.apache.org/r/69599/#comment297169> Doesn't look like you would need mesos::internal here since we're already in that namespace, did you find you had to add it? src/master/allocator/sorter/drf/sorter.hpp Line 444 (original), 446 (patched) <https://reviews.apache.org/r/69599/#comment297170> Ditto here. src/master/allocator/sorter/drf/sorter.cpp Lines 661-663 (original), 661-662 (patched) <https://reviews.apache.org/r/69599/#comment297163> Why the change to this line? Let's keep the diff small and relevant to this change only src/master/allocator/sorter/drf/sorter.cpp Lines 670-676 (original), 669-677 (patched) <https://reviews.apache.org/r/69599/#comment297164> Not for this patch, but I'm just noticing this code seems easier to understand if s/scalar/total/ The if condition is a bit puzzling to read, maybe it would be clearer written as follows: ``` foreachpair (const string& resourceName, const Value::Scalar& total, total_.totals) { if (total.value() <= 0.0) { continue; } // Filter out the resources excluded from fair sharing. if (fairnessExcludeResourceNames.isSome() && fairnessExcludeResourceNames->count(resourceName) > 0) { continue; } Value::Scalar allocation = node->allocation.totals .get(resourceName) .getOrElse(Value::Scalar()); // Default to zero. share = std::max(share, allocation / total.value()); } ``` src/master/allocator/sorter/drf/sorter.cpp Lines 671-675 (original), 670-676 (patched) <https://reviews.apache.org/r/69599/#comment297168> The following seems simpler to read? ``` Value::Scalar allocation = node->allocation.totals .get(resourceName) .getOrElse(Value::Scalar()); // Default to zero. share = std::max(share, allocation.value() / total.value()); ``` src/master/allocator/sorter/random/sorter.hpp Line 176 (original), 178 (patched) <https://reviews.apache.org/r/69599/#comment297161> Ditto here. src/master/allocator/sorter/random/sorter.hpp Line 420 (original), 422 (patched) <https://reviews.apache.org/r/69599/#comment297162> Ditto here. - Benjamin Mahler On Jan. 4, 2019, 8: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, 8: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 > >
