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 18175: 'Try' cleanups in mesos.
Date Mon, 17 Feb 2014 06:32:20 GMT

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

Ship it!



src/log/replica.cpp
<https://reviews.apache.org/r/18175/#comment64802>

    I am a little worried here because the 'learned' set could be quite large. Copying the
entire set maybe quite expensive.
    
    I don't think gcc can get rid of the copying here as state.get().learned is an lvalue.
    
    What about leaving a NOTE here?



src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/18175/#comment64806>

    Unfortunately, this looks a little weird as a result of returning const T&.
    
    I am thinking of providing a 'const' version of operator [] in hashmap:
    
    const value_type& operator [] (const key_type& key) const;
    
    And this function internally provides a CHECK to make sure that the key exists.
    
    However, I am still a little hesitate because the [] semantics for const and non-const
versions are different if we provide the overload, which could be quite misleading to the
user.


- Jie Yu


On Feb. 16, 2014, 11:38 p.m., Ben Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18175/
> -----------------------------------------------------------
> 
> (Updated Feb. 16, 2014, 11:38 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Jie Yu, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-1008
>     https://issues.apache.org/jira/browse/MESOS-1008
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See: https://reviews.apache.org/r/18173/
> 
> I also killed instances of our old pattern:
>   
>   const Try<T>& t = foo();
> 
>   // Replaced with:
>   Try<T> t = foo();
> 
> I did this for two reasons:
>   1. gcc is often already eliding the copy.
>   2. This will be obviated when we add move constructors to Try, since foo() is returning
an rvalue.
> 
> 
> Diffs
> -----
> 
>   src/log/replica.cpp 746d6c35c9255775ab6e70b0daf1dcecf63c16a0 
>   src/master/contender.cpp e3b0737195aba42c805a05c96b054cec1471b502 
>   src/messages/messages.hpp 98038c017a7d02c7c4f04f91c89ca56f566f707b 
>   src/slave/containerizer/isolators/cgroups/cpushare.cpp 989d3848343d4255c25796aa5be81dbb93a29b6e

>   src/slave/containerizer/isolators/cgroups/mem.cpp a01e114c168b820e6c54af9257716b753940d510

>   src/slave/slave.cpp 8ad955a71957421ae51e1becc4b06a4b6b091eb2 
>   src/slave/state.cpp 9af6c5b0582e972523028d226703070293b92f8b 
>   src/tests/cgroups_tests.cpp f0dead7c09fe3cb4355c5b3b4603a2cb8c2cd3d5 
>   src/tests/mesos.cpp 98333a7d3a2c7c8004aacc43dda3add31d5b5487 
>   src/tests/values_tests.cpp c72dbc2ec05e12541c03196577dff3561bff08ed 
>   src/tests/zookeeper_url_tests.cpp b143c539530e3319f603d9276ebf84dfecfcf6f2 
> 
> Diff: https://reviews.apache.org/r/18175/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>


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