mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jie Yu" <yujie....@gmail.com>
Subject Re: Review Request 30911: Updated the filter abstraction for Resources.
Date Mon, 16 Feb 2015 23:43:42 GMT

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



include/mesos/resources.hpp
<https://reviews.apache.org/r/30911/#comment118713>

    We avoid using typedefs as much as possible trying to be more explicit.



include/mesos/resources.hpp
<https://reviews.apache.org/r/30911/#comment118715>

    See my comments below. Can we punt this for now and leave a TODO for supporing filtering
for RepeatedPtrField?
    
    This struct itself is confusing because we have `reserved` as well.
    
    ```
    Resources::Reserved("role1")
    ```
    vs
    ```
    resources.reserved("role1")
    ```
    
    I think being explicit and using lambda::bind is not a bad thing. As least the readers
immediately know that `filter` takes a predicate function. What do you think?



include/mesos/resources.hpp
<https://reviews.apache.org/r/30911/#comment118716>

    I don't think we should remove these two helper functions as they are quite convinent
to use. (just like we still have empty() in std containers even if we can use size() == 0
to replace it).



include/mesos/resources.hpp
<https://reviews.apache.org/r/30911/#comment118714>

    Can we make `filter` function a member of `Resources` instead. We usually prefer member
functions than global functions. Also, looking at the callsites, this version does not read
well. For example:
    ```
    filter(Resources::Reserved("role2"), slave2.resources())
    ```
    
    It's not obvious what does this function do. For instance, Readers might think that `Reserved("role2")`
is a function which returns all resources reserved for `"role2"`.
    
    I understand that you want to make filter seamlessly work for RepeatedPtrField. Let's
punt that for now and leave a TODO. For the above case, simply do the following for now (i.e.,
still keep `reserved` and `unreserved`).
    ```  
    Resources(slave2.resourecs()).reserved("role2");
    ```


- Jie Yu


On Feb. 14, 2015, 12:11 a.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30911/
> -----------------------------------------------------------
> 
> (Updated Feb. 14, 2015, 12:11 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-2348
>     https://issues.apache.org/jira/browse/MESOS-2348
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See [JIRA Ticket](https://issues.apache.org/jira/browse/MESOS-2348).
> 
> 
> Diffs
> -----
> 
>   include/mesos/resources.hpp c7cc46e0183ea97013dd088a717da6c0e6ed5cf0 
>   src/common/resources.cpp 98371f6873482d0cdbefeb279b58ae6cc680a88f 
>   src/master/allocator/mesos/hierarchical.hpp 2680d6231927867d5a8d75cbc42b81d6c75fc7f2

>   src/master/master.hpp 6a39df04514c756415354fae66c5835ada191c52 
>   src/master/validation.cpp acc35b25c93f2d3900d79c3070b1d681416ed66b 
>   src/slave/slave.cpp ec7ec1356e745bb07484ae1755c9183b038043b3 
>   src/tests/hierarchical_allocator_tests.cpp eeecfb64540b16666915074aaffaa5d506b203bc

>   src/tests/resources_tests.cpp 3f98782fd437dba808d720bf8e9b94b8fa7e0feb 
> 
> Diff: https://reviews.apache.org/r/30911/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>


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