> On Oct. 15, 2014, 1:25 a.m., Vinod Kone wrote: > > src/master/master.cpp, lines 3146-3149 > > <https://reviews.apache.org/r/25525/diff/8/?file=717605#file717605line3146> > > > > Hmm. i don't think we want these semantics. Why not just fail if > > readmission failed? > > Cody Maloney wrote: > So if the registrar actually fails to readmit the slave, then the master > will go down on a LOG(FATAL). In the case where we couldn't readmit the slave > for some reason (Ex: the registrar thought the upgrade was incompatible, hit > the fall-through case in master/master.hpp), then the slave will be sent a > shutdown message and explicitly removed at that point. > > So I think the semantics you want are there / the comment can just be > removed. > > Vinod Kone wrote: > i think "we just don't see..." threw me off. instead say that "readmit > will fail leading to slave shutdown.". > > killing the comment is fine too.
Comment removed > On Oct. 15, 2014, 1:25 a.m., Vinod Kone wrote: > > src/common/slaveinfo_utils.cpp, lines 64-96 > > <https://reviews.apache.org/r/25525/diff/8/?file=717601#file717601line64> > > > > 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. > > Cody Maloney wrote: > So I can do that, which gives: > ```c++ > if (!(newSlaveInfo.id() == oldSlaveInfo.id())) { > return Error("id cannot be changed (old: " + > stringify(oldSlaveInfo.id()) + > " new: " + stringify(newSlaveInfo.id()) + ")"); > } > > if (newSlaveInfo.hostname() != oldSlaveInfo.hostname()) { > return Error("hostname cannot be changed (old: " + > stringify(oldSlaveInfo.hostname()) + " new: " + > stringify(newSlaveInfo.hostname()) + ")"); > } > > if (newSlaveInfo.checkpoint() != oldSlaveInfo.checkpoint()) { > return Error("checkpoint cannot be changed (old: " + > stringify(oldSlaveInfo.checkpoint()) + " new: " + > stringify(newSlaveInfo.checkpoint()) + ")"); > } > > if (newSlaveInfo.port() != oldSlaveInfo.port()) { > return Error("port cannot be changed (old: " + > stringify(oldSlaveInfo.port()) + " new: " + > stringify(newSlaveInfo.port()) + ")"); > } > ``` > > My problems with it are: > 1. There is a lot of redundant code which needs to be mentally parsed. I > have to realize each of those is the same reading 3 lines of code before I > can recognize "oh, we're checking over and over again these two fields in the > different versions are the same, and mentally condense the code away" > 2. How likely is it that you would actually notice all the new vs. old if > I got one of the error messages wrong? What about if I made any of a large > number of possible slight typos in one vs. the next? What if I compare two > different string fields? The compiler can't catch any of these or help me > with them. > > > ```c++ > checkMembersUnchanged(newSlaveInfo, oldSlaveInfo, > make_pair("SlaveID", &SlaveInfo::id), > make_pair("hostname", &SlaveInfo::hostname), > make_pair("checkpoint", &SlaveInfo::checkpoint), > make_pair("port", &SlaveInfo::port)); > ``` > > Where when reading just that code, I see I'm giong to check that the > members are unchanged, probably between newSlaveInfo and oldSlaveInfo. That > the fields are "SlaveID", "hostname", "checkpoint", and "port". > > > The checkMembersUnchanged takes a little bit more time/effort to > recognize, esp. since it is c++11 variadic templates, recursion, and pointers > to member functions + calling pointers to member functions (Which is some of > the oddest syntax in C++). What is nice about it though is: > > 1. The function though guarantees I call the same function on both old > and new / access the same member > 2. Formats the error string consistently, gives me one place to update / > change it if needed > 3. Makes one place for the ordering to be correct. > > Vinod Kone wrote: > If you look around the code base, we prefer approach 1 to 2. It's really > not that hard to follow that you are doing the same thing. Just adding a > comment on top would also help guide the users. > > For readability, I would also suggest putting "(old: " on the next line > and "new: " on the next line. > > With 2) my problem is that it is too esoteric, both the name and the > arguments. Updated to have the checks written out. The reason why the codebase has all the checks written out currently is because that was the only option. It is defintely possible to write a generic "check equal" helper which would apply in all those cases, and give them all exactly the same shape, while making their definitions more concise. I'll leave that for later work though. - Cody ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25525/#review56590 ----------------------------------------------------------- On Oct. 21, 2014, 11:55 p.m., Cody Maloney wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25525/ > ----------------------------------------------------------- > > (Updated Oct. 21, 2014, 11:55 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 c44a9ad47d6e1262949b9049f4ae25b049440d99 > 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 18898e9e1897b9d8cecd10d0ef7f25540cc9916d > src/master/master.cpp be910d9f02405e25e5ad6ae3bf13d770a0c2b417 > src/slave/slave.cpp 7b5474ae898f369fc55670cf1dbbee684bc0ae4b > src/tests/attributes_tests.cpp 240a8cac18ac12178cf73e8eeb88bd50e3fcc03b > src/tests/mesos.hpp e40575c6d330cfa3fbfad2efcf601418c413b992 > src/tests/slave_tests.cpp 759670ad91c2f8afbd4b9dcb2f367f50482358e9 > > Diff: https://reviews.apache.org/r/25525/diff/ > > > Testing > ------- > > make check on localhost > > > Thanks, > > Cody Maloney > >
