----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25525/#review53248 -----------------------------------------------------------
src/Makefile.am <https://reviews.apache.org/r/25525/#comment92804> please keep this in alphabetical order src/common/attributes.hpp <https://reviews.apache.org/r/25525/#comment92805> i don't think these comments are necessary. the interface is very clear. src/common/slaveinfo_utils.hpp <https://reviews.apache.org/r/25525/#comment92816> consider putting this in a namespace. at least mesos, probably mesos::slaveinfo_utils. src/common/slaveinfo_utils.hpp <https://reviews.apache.org/r/25525/#comment92806> nit: s/new_/new/ src/common/slaveinfo_utils.cpp <https://reviews.apache.org/r/25525/#comment92810> these could be in an anonymous namespace too, which i think is preferable. src/common/slaveinfo_utils.cpp <https://reviews.apache.org/r/25525/#comment92807> make sure it's clear this depends on the variadic template check in configure. src/common/slaveinfo_utils.cpp <https://reviews.apache.org/r/25525/#comment92813> std::pair<const std::string&, const T*> might be preferable. src/common/slaveinfo_utils.cpp <https://reviews.apache.org/r/25525/#comment92809> the operator precedence here is a little tricky - can you simplify it a bit? or maybe make it clear that T is actually a T* in the API. src/common/slaveinfo_utils.cpp <https://reviews.apache.org/r/25525/#comment92811> is there a reason you're taking copies instead of using const& throughout? src/master/master.hpp <https://reviews.apache.org/r/25525/#comment92814> nit: s/propogate/propagate/ src/master/master.hpp <https://reviews.apache.org/r/25525/#comment92815> nit - keep the = on the previous line. src/master/master.cpp <https://reviews.apache.org/r/25525/#comment92817> are these tabs? src/master/master.cpp <https://reviews.apache.org/r/25525/#comment92819> i wonder if it's worth splitting this method into two wrappers around a _readdSlave method to avoid the boolean parameter. src/master/master.cpp <https://reviews.apache.org/r/25525/#comment92820> tab src/tests/attributes_tests.cpp <https://reviews.apache.org/r/25525/#comment92821> nit: Attributes a = Attributes::parse( "cpus:45.55; ports:[10000-20000, 30000-50000];"); src/tests/attributes_tests.cpp <https://reviews.apache.org/r/25525/#comment92822> EXPECT_FALSE(b.isSubsetOf(a)); etc? src/tests/slave_tests.cpp <https://reviews.apache.org/r/25525/#comment92823> test for failure to recover with a subset? - Dominic Hamon On Sept. 12, 2014, 4:33 p.m., Cody Maloney wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25525/ > ----------------------------------------------------------- > > (Updated Sept. 12, 2014, 4:33 p.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, Patrick Reilly, and Vinod > Kone. > > > Bugs: MESOS-1739 > https://issues.apache.org/jira/browse/MESOS-1739 > > > Repository: mesos-git > > > Description > ------- > > Allows attributes and resources to be set to a superset of what they were > previously on a slave restart. > > Incorporates all comments from: > https://issues.apache.org/jira/browse/MESOS-1739 > and the former review request: > https://reviews.apache.org/r/25111/ > > > Diffs > ----- > > src/Makefile.am 9b973e5 > src/common/attributes.hpp 0a043d5 > src/common/attributes.cpp aab114e > src/common/slaveinfo_utils.hpp PRE-CREATION > src/common/slaveinfo_utils.cpp PRE-CREATION > src/master/master.hpp b492600 > src/master/master.cpp d5db24e > src/slave/slave.cpp 1b3dc73 > src/tests/attributes_tests.cpp 240a8ca > src/tests/slave_tests.cpp 69be28f > > Diff: https://reviews.apache.org/r/25525/diff/ > > > Testing > ------- > > make check on localhost > > > Thanks, > > Cody Maloney > >
