mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 69599: Pulled up a new class `ResourceQuantities`.
Date Fri, 04 Jan 2019 21:47:41 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message