mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 28089: Refactored operators for Resource object and made them private.
Date Mon, 17 Nov 2014 22:12:52 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28089/#review61802
-----------------------------------------------------------

Ship it!


Modulo comments.

In tandem with this change, we need to get away from our code merging 'Resources' objects
across slaves, since the resources are inherently namespaced! For example, summing the ports
across all slaves is not useful for accounting purposes.


include/mesos/resources.hpp
<https://reviews.apache.org/r/28089/#comment103702>

    Can you put implicit on the same line? Like future.hpp, for example.



src/common/resources.cpp
<https://reviews.apache.org/r/28089/#comment103731>

    How about 'subtractable' and 'addable' instead of 'combinable' and 'removable', seems
a bit more direct into what these are to be used for?
    
    Be sure to update the comments. :)



src/common/resources.cpp
<https://reviews.apache.org/r/28089/#comment103728>

    Let's avoid saying superset now, let's just say contains :)
    
    How about:
    
    ```
    // NOTE: Set substraction is always well defined, it does not
    // require 'right' to be contained within 'left'.
    ```



src/common/resources.cpp
<https://reviews.apache.org/r/28089/#comment103729>

    Let's stop saying superset :)
    
    How about:
    
    // Returns iff 'right' is contained in 'left'.
    
    However, shouldn't this comment be in the header?



src/common/resources.cpp
<https://reviews.apache.org/r/28089/#comment103721>

    How about:
    
    ```
    if (left.name() != right.name() ||
        left.type() != right.type() ||
        left.role() != right.role()) {
      return false;
    }
    ```
    
    We've been avoiding these one-liners I think.



src/common/resources.cpp
<https://reviews.apache.org/r/28089/#comment103722>

    Can this one be removed? Is it used now that it is hidden in the .cpp file?



src/common/resources.cpp
<https://reviews.apache.org/r/28089/#comment103723>

    Hm.. I assume you removed the call to `matches()` intentionally because of some non-local
reasoning? Why is it safe to assume that the name, type, and role match?
    
    If you really don't want to check matching, we should at least have a comment! Ditto for
-=, hard to tell locally why it doesn't check the name and role.



src/common/resources.cpp
<https://reviews.apache.org/r/28089/#comment103726>

    Can you add some TODO's to leverage the += and -= operators within these? You'll have
to fix the bugs in the Values operators first, but dropping a TODO for context in this patch
would be great :)



src/common/resources.cpp
<https://reviews.apache.org/r/28089/#comment103732>

    CopyFrom implicitly does Clear() + MergeFrom(), so could these be simplified?
    
    ```
    left.mutable_scalar()->CopyFrom(left.scalar() + right.scalar());
    ```
    
    Ditto for all the others, looks like they could all be done in one line, ditto for -=
implementation.


- Ben Mahler


On Nov. 17, 2014, 8:34 p.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28089/
> -----------------------------------------------------------
> 
> (Updated Nov. 17, 2014, 8:34 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-1974
>     https://issues.apache.org/jira/browse/MESOS-1974
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Hide operators for Resource object. Split "matches" to handle future first class infos.
> 
> 
> Diffs
> -----
> 
>   include/mesos/resources.hpp 0e37170262d3470570a3436b7835bb1d4a121056 
>   src/common/resources.cpp e9a0c85dc3748aa635843d98cd5993d5648ec5c2 
>   src/tests/resources_tests.cpp 3e50889ff2433930bd137a9d836d0a8bfc2d0388 
> 
> Diff: https://reviews.apache.org/r/28089/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>


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