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 the resource quantities class for more general use.
Date Mon, 07 Jan 2019 05:14:05 GMT

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


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