----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25525/#review53100 -----------------------------------------------------------
OK. I went through parts of this review but I have a bigger suggestion in mind, before I get too much into the weeds. I think it's worthwhile for you to write up a design doc similar to the framework info doc w.r.t. updating SlaveInfo. This will force you to think about the repercussions of changing each of the SlaveInfo fields on the Mesos stack (master/allocator/slave/tasks/executors). Are you game? src/common/attributes.cpp <https://reviews.apache.org/r/25525/#comment92503> 2 blank lines between outer elements. src/common/attributes.cpp <https://reviews.apache.org/r/25525/#comment92506> Why not just add "<=" and ">=" operator overloads like we did for Resources instead of adding subset/superset methods? Also, please add tests for these in attributes_tests.cpp. src/common/attributes.cpp <https://reviews.apache.org/r/25525/#comment92504> 2 blank lines between outer elements. src/common/attributes.cpp <https://reviews.apache.org/r/25525/#comment92505> 2 blank lines between outer elements. src/common/slaveinfo_utils.hpp <https://reviews.apache.org/r/25525/#comment92510> I think this function name is misleading. It is not reconfiguring a slave, it is just doing a compatibility test. Try<bool> isCompatible( const mesos::SlaveInfo& newInfo, const mesos::SlaveInfo& oldInfo); You should also add a comment on what this method is doing, when is it considered compatible etc. src/common/slaveinfo_utils.cpp <https://reviews.apache.org/r/25525/#comment92514> Typo? This sentence is incomplete. src/common/slaveinfo_utils.cpp <https://reviews.apache.org/r/25525/#comment92515> Please use camelCase instead of snake case for variable names. There are only few exceptions in the code base to this rule, but they usually come with a good reason. s/attributes_new/newAttributes/ s/resources_new/newResources/ s/resources_old/oldResources/ src/common/slaveinfo_utils.cpp <https://reviews.apache.org/r/25525/#comment92516> you can compare the slave ids directly, instead of strings, if you include common/type_utils.hpp Error("SlaveID cannot be changed (old: " + stringify(old.id()) + ", new: " + stringify(new.id()) + ")") src/common/slaveinfo_utils.cpp <https://reviews.apache.org/r/25525/#comment92535> I think we have to be careful about allowing 'checkpoint' to be changed. If you haven't already, please refer to the design doc for updating FrameworkInfo (https://docs.google.com/document/d/1vEBuFN9mm3HkrNCmkAuwX-4kNYv0MwjUecLE3Jp_BqE/edit?usp=sharing) which talks about checkpointing. In particular what are the semantics for already running tasks/executors if 'checkpoint' is changed? src/common/slaveinfo_utils.cpp <https://reviews.apache.org/r/25525/#comment92522> also include the new and old attributes in the error message to ease debugging. src/common/slaveinfo_utils.cpp <https://reviews.apache.org/r/25525/#comment92523> new line. also, include the old and new resources in the error message. src/common/slaveinfo_utils.cpp <https://reviews.apache.org/r/25525/#comment92532> How about: ``` Try<Nothing> isCompatible(const SlaveInfo& newSlaveInfo, const SlaveInfo& oldSlaveIfno) { // .. if (!(newSlaveInfo.id() == oldSlaveInfo.id())) { return Error(...); } //... if (newSlaveInfo.hostname() != oldSlaveInfo.hostname()) { return Error(...); } //... if (newSlaveInfo.resources() < oldSlaveInfo.resources()) { return Error(...); } //... if (newSlaveInfo.attributes() < oldSlaveInfo.attributes()) { return Error(...); } return Nothing(); } ``` src/slave/slave.cpp <https://reviews.apache.org/r/25525/#comment92520> Why is this function returning a Try<bool> instead of Try<Nothing>? I don't see how it is using the boolean? Also, s/res/compatible/ src/slave/slave.cpp <https://reviews.apache.org/r/25525/#comment92534> I don't think there's much value left with logging the old and new slave info. The res.error() will pinpoint what the error is. src/slave/slave.cpp <https://reviews.apache.org/r/25525/#comment92521> You should checkpoint the updated slave info! - Vinod Kone On Sept. 11, 2014, 8:13 a.m., Cody Maloney wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25525/ > ----------------------------------------------------------- > > (Updated Sept. 11, 2014, 8:13 a.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/slave_tests.cpp 69be28f > > Diff: https://reviews.apache.org/r/25525/diff/ > > > Testing > ------- > > make check on localhost > > > Thanks, > > Cody Maloney > >
