----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69599/#review211714 -----------------------------------------------------------
src/common/resource_quantities.hpp Lines 81 (patched) <https://reviews.apache.org/r/69599/#comment297241> This TODO looks like it belongs elsewhere, like at the top or after this function, or just remove it? src/common/resource_quantities.cpp Lines 37 (patched) <https://reviews.apache.org/r/69599/#comment297242> Would be nice to have a comment here that this is based on fromSimpleString so that anyone reading it and wondering why it's done this way knows, as well as if they try to change the implementation they'll think about that. Right now it's only mentioned in the header which is unlikely to be spotted when looking at the implementation. src/common/resource_quantities.cpp Lines 45 (patched) <https://reviews.apache.org/r/69599/#comment297243> Why not just stick with "Failed to parse " like the other messages? ``` "Failed to parse '" + token + "'" ": expected 1 ':' but found " + stringify(pair.size()); ``` src/common/resource_quantities.cpp Lines 56-63 (patched) <https://reviews.apache.org/r/69599/#comment297245> Can you close the quote on the same line for these? Also, seems more readable for these if the second line is reason only? ``` "Failed to parse '" + pair[1] + "' to quantity:" " only scalar values are allowed"); ``` src/master/allocator/sorter/drf/sorter.cpp Line 671 (original), 671 (patched) <https://reviews.apache.org/r/69599/#comment297248> Why do you say to consider this, we actually don't do anything to ensure this AFAIK, so it would fail, no? src/master/allocator/sorter/drf/sorter.cpp Lines 675-676 (original), 678-681 (patched) <https://reviews.apache.org/r/69599/#comment297246> I find the if condition here adds unnecessary load on the reader, there isn't really anything special about 0 to warrant a special case? E.g. ``` Value::Scalar allocation = node->allocation.totals .get(resourceName) .getOrElse(Value::Scalar()); // Absent means zero. share = std::max(share, allocation.value() / total.value()); ``` If this is a performance optimization, can you show it's helpful? It looks like this comment was missed in the earlier review? src/master/allocator/sorter/drf/sorter.cpp Lines 680 (patched) <https://reviews.apache.org/r/69599/#comment297247> As a general rule I would suggest using CHECK_NOTNONE in cases where we do unguarded retrieval, e.g. ``` CHECK_NOTNONE(Resources::parse("cpus:1")) ``` In cases where the logic around whether it's some is complex and we want to sanity check it, probably CHECK_SOME is better: ``` // Many cases but should always lead to some CHECK_SOME(v); some code v->foo(); ``` But in cases like this, where we're actually inside the guarded block, just use -> ``` if (allocation.isSome()) { share = std::max(share, allocation->value() / scalar.value()); ``` - Benjamin Mahler On Jan. 5, 2019, 11:28 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69599/ > ----------------------------------------------------------- > > (Updated Jan. 5, 2019, 11:28 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/4/ > > > Testing > ------- > > make check > Dedicated test in r/69600 > > > Thanks, > > Meng Zhu > >
