> On Sept. 11, 2014, 9:53 p.m., Vinod Kone wrote: > > 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?
I'll work on a design doc later today. Will let you know when I have it. > On Sept. 11, 2014, 9:53 p.m., Vinod Kone wrote: > > src/slave/slave.cpp, line 3109 > > <https://reviews.apache.org/r/25525/diff/2/?file=685078#file685078line3109> > > > > 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/ The bool is indicating whether or not anything changed. It is quite likely / possible that there won't be any change to the slaveInfo in which case we don't need to do anything special / fall into the old code path. This is relevant a lot more in the server than the slave. If the slave was allowing runtime updates it would be more critical there. > On Sept. 11, 2014, 9:53 p.m., Vinod Kone wrote: > > src/common/attributes.cpp, lines 161-172 > > <https://reviews.apache.org/r/25525/diff/2/?file=685073#file685073line161> > > > > 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. The style guide says we shouldn't overload those: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Operator_Overloading. There isn't an exception in the mesos C++ style guide yet (Perhaps one should be added)? For me it's not intuitive unless the type I'm working with that <= and >= do superset/subset. I had to read through the resource code to verify it was doing a subset/superset with <= (There is no >=, >, < on resources). Also note when giving these different behaviors, sometimes what people take to be true of operators (<= should return the exact opposite of >). > On Sept. 11, 2014, 9:53 p.m., Vinod Kone wrote: > > src/common/slaveinfo_utils.hpp, line 26 > > <https://reviews.apache.org/r/25525/diff/2/?file=685074#file685074line26> > > > > 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. Changed the function name. I don't want to write the compatibility guarantees in the header because somewhere down the line someone will come and update the function body but not see / change the header comment (And it would be hard to spot in a code review). The code is fairly readable for what is checked. > On Sept. 11, 2014, 9:53 p.m., Vinod Kone wrote: > > src/common/slaveinfo_utils.cpp, line 48 > > <https://reviews.apache.org/r/25525/diff/2/?file=685075#file685075line48> > > > > 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? Updated so that only attributes and resources can be changed. Everything else now must stay exactly the same. > On Sept. 11, 2014, 9:53 p.m., Vinod Kone wrote: > > src/common/slaveinfo_utils.cpp, line 66 > > <https://reviews.apache.org/r/25525/diff/2/?file=685075#file685075line66> > > > > 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(); > > } > > ``` It's important to note true / false for whether or not their are changes (It changes what code path we take in the server). In the case of an eventual API endpoint for updating slave info it will be critical to know and log if something sends us the same slaveInfo struct as we had before and not create excess noise running around updating the slaveInfo on the master. Also note attributes and resources we are checking for just superset, not a proper superset. Attributes only has operator <= defined. SlaveID is only ==, there is a general lack of robustness around defining all the related operators. > On Sept. 11, 2014, 9:53 p.m., Vinod Kone wrote: > > src/slave/slave.cpp, line 3122 > > <https://reviews.apache.org/r/25525/diff/2/?file=685078#file685078line3122> > > > > You should checkpoint the updated slave info! info contains the new slave info set at the command line. State contains the old / from checkpoint info. This effectively used to set info to be what was in the checkpoint file which didn't actually change anything because the two were identical. - Cody ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25525/#review53100 ----------------------------------------------------------- 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 > >
