----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25525/#review56590 -----------------------------------------------------------
src/common/slaveinfo_utils.hpp <https://reviews.apache.org/r/25525/#comment96944> s/legal/compatible/ Also mention when this returns a false vs error. src/common/slaveinfo_utils.cpp <https://reviews.apache.org/r/25525/#comment96960> Hmm. This is still hard to follow. Using variadic templated function here doesn't make it any simpler to read/follow. Infact it makes it harder because now I need to read understand what the template function does. I think explicitly checking each field (as suggested in the earlier comment) reads better. src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/25525/#comment96962> Instead of adding a 'update()' method on Slave struct, just manipulate the resources here. Also, you want to CHECK that the new resources are more than the old resources. src/master/master.hpp <https://reviews.apache.org/r/25525/#comment96965> Can you rephrase this? It's hard to follow. src/master/master.hpp <https://reviews.apache.org/r/25525/#comment97014> This should be CHECK_SOME(compatible). src/master/master.cpp <https://reviews.apache.org/r/25525/#comment96981> ``` LOG(WARNING) << "Slave " << *slave << " attempted to re-register with incompatible info: " << compatible.error() << "; shutting it down" ``` Also, maybe we don't need to explicitly 'removeSlave(slave)' here? If a compatible slave is not re-registered within timeout, master will automatically remove it. src/master/master.cpp <https://reviews.apache.org/r/25525/#comment96982> include "compatible.error()" in the message. src/master/master.cpp <https://reviews.apache.org/r/25525/#comment97010> s/Replicate the updated attribute to the other masters/Readmit the slave with updated slave info/ Also, period at the end of comments please. src/master/master.cpp <https://reviews.apache.org/r/25525/#comment97011> Hmm. i don't think we want these semantics. Why not just fail if readmission failed? src/master/master.cpp <https://reviews.apache.org/r/25525/#comment97019> s/__reregisterSlaveExistingMaster/__reregisterUpdatedSlave/ src/master/master.cpp <https://reviews.apache.org/r/25525/#comment97018> This seems very unintuitve for users/operators. Lets fix this in this patch rather than doing a TODO. Also, AFAICT, the changes are not lost because both the registrar and the allocator are informed about the new slave info. Am I missing something? src/master/master.cpp <https://reviews.apache.org/r/25525/#comment96973> 2 blank lines. src/master/master.cpp <https://reviews.apache.org/r/25525/#comment96974> 2 blank lines. src/slave/slave.cpp <https://reviews.apache.org/r/25525/#comment96969> We can kill the NOTE above because now we set the 'id' field because 'info' is going to be the updated slave info that slave uses. src/slave/slave.cpp <https://reviews.apache.org/r/25525/#comment96970> End statements with a period. s/slaveInfo/slave info/ src/slave/slave.cpp <https://reviews.apache.org/r/25525/#comment97017> Consider: ``` if (flags.checkpoint && compatible.get()) { const string& path = paths::getSlaveInfoPath(metaDir, info.id()); LOG(INFO) << "Checkpointing updated SlaveInfo to ' << path << "'"; CHECK_SOME(state::checkpoint(path, info)); } ``` src/tests/attributes_tests.cpp <https://reviews.apache.org/r/25525/#comment96966> 2 blank lines between outer elements. src/tests/mesos.hpp <https://reviews.apache.org/r/25525/#comment96967> 2 blank lines. src/tests/mesos.hpp <https://reviews.apache.org/r/25525/#comment96968> 2 blank lines. src/tests/slave_tests.cpp <https://reviews.apache.org/r/25525/#comment97021> 2 blank lines. src/tests/slave_tests.cpp <https://reviews.apache.org/r/25525/#comment97022> Any particular reason you want to explicitly wait for slave registration? FWIW, the AWAIT on offers below will guarantee that slave has registered. src/tests/slave_tests.cpp <https://reviews.apache.org/r/25525/#comment97023> s/new_offers/newOffers/ we use camel case for variables in the code base as much as possible. src/tests/slave_tests.cpp <https://reviews.apache.org/r/25525/#comment97024> s/recovered_slave/recoveredSlave/ src/tests/slave_tests.cpp <https://reviews.apache.org/r/25525/#comment97025> ditto. Why explicitly wait for recovery? Reception of new offers guarantees that. - Vinod Kone On Oct. 10, 2014, 6:49 p.m., Cody Maloney wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25525/ > ----------------------------------------------------------- > > (Updated Oct. 10, 2014, 6:49 p.m.) > > > Review request for mesos, Adam B 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 d503c8df73cda15a9d59254e8265e4a5d0e003a4 > src/common/attributes.hpp 0a043d5b5dca804c6dd215cabd2704f24df71a33 > src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 > src/common/slaveinfo_utils.hpp PRE-CREATION > src/common/slaveinfo_utils.cpp PRE-CREATION > src/master/allocator.hpp 02d20d0cc802805bc702891306aa42894531b223 > src/master/hierarchical_allocator_process.hpp > 31dfb2cc86e2e74da43f05fbada10976ce65f3e4 > src/master/master.hpp 14f1d0fd240c9cd0718d0238e1fbb9c733190205 > src/master/master.cpp cb46cec0674b3aa031450c5b4f48f4f8bb92767d > src/slave/slave.cpp cb3759993f863590cb1545c73072feb0331aa6c9 > src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b > src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 > src/tests/slave_tests.cpp 69be28f6e82b99e23424bd2be8294f715d8040d4 > > Diff: https://reviews.apache.org/r/25525/diff/ > > > Testing > ------- > > make check on localhost > > > Thanks, > > Cody Maloney > >
