> On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote: > > src/common/slaveinfo_utils.cpp, line 44 > > <https://reviews.apache.org/r/25525/diff/3/?file=688208#file688208line44> > > > > std::pair<const std::string&, const T*> might be preferable.
T is a pointer to member function in this case. I could try specifying it more exactly as a type, but const T* isn't it. There were also some issues with type deduction in variadic templates if I recall correctly. Making it so it is vague/general 'T' rather than the pointer to member function specifically makes the code simpler to write. > On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote: > > src/common/slaveinfo_utils.cpp, line 47 > > <https://reviews.apache.org/r/25525/diff/3/?file=688208#file688208line47> > > > > 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. There is only one operator there. It's calling a pointer to member function. It is one of those C++ things you really don't do often. > On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote: > > src/common/slaveinfo_utils.cpp, line 64 > > <https://reviews.apache.org/r/25525/diff/3/?file=688208#file688208line64> > > > > is there a reason you're taking copies instead of using const& > > throughout? In this case we're constructing Attributes() around the protobuf object which forces a copy into the new class. Same goes for the resources ones. Inside the variadic template we're dealing with results of functions where value is what we want > On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote: > > src/master/master.hpp, line 927 > > <https://reviews.apache.org/r/25525/diff/3/?file=688209#file688209line927> > > > > nit: s/propogate/propagate/ fixed > On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote: > > src/master/master.cpp, line 3032 > > <https://reviews.apache.org/r/25525/diff/3/?file=688210#file688210line3032> > > > > are these tabs? They definitely aren't in the new version > On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote: > > src/master/master.cpp, line 4222 > > <https://reviews.apache.org/r/25525/diff/3/?file=688210#file688210line4222> > > > > i wonder if it's worth splitting this method into two wrappers around a > > _readdSlave method to avoid the boolean parameter. There is a restructuring in general of the recover logic which should happen at some point (I was avoiding it in this patchset to minimize jitter). The _reregisterSlaveExistingMaster for instance is very similar to _reregisterSlave, and readdSlave sometimes has the allocator in it and sometimes outside (It has three call sites now). > On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote: > > src/tests/attributes_tests.cpp, line 70 > > <https://reviews.apache.org/r/25525/diff/3/?file=688212#file688212line70> > > > > nit: > > > > Attributes a = Attributes::parse( > > "cpus:45.55; ports:[10000-20000, 30000-50000];"); fixed (although all the other tests in the file do it the other way) > On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote: > > src/tests/attributes_tests.cpp, line 76 > > <https://reviews.apache.org/r/25525/diff/3/?file=688212#file688212line76> > > > > EXPECT_FALSE(b.isSubsetOf(a)); etc? added > On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote: > > src/tests/slave_tests.cpp, line 995 > > <https://reviews.apache.org/r/25525/diff/3/?file=688213#file688213line995> > > > > test for failure to recover with a subset? There are a couple more test cases which should be added (make sure the new SlaveInfo is checkpointed, the changes persist after master failover) > On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote: > > src/master/master.hpp, line 1198 > > <https://reviews.apache.org/r/25525/diff/3/?file=688209#file688209line1198> > > > > nit - keep the = on the previous line. fixed > On Sept. 12, 2014, 11:48 p.m., Dominic Hamon wrote: > > src/Makefile.am, line 253 > > <https://reviews.apache.org/r/25525/diff/3/?file=688204#file688204line253> > > > > please keep this in alphabetical order fixed - Cody ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25525/#review53248 ----------------------------------------------------------- On Sept. 12, 2014, 11: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, 11: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 > >
