----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25111/#review51753 -----------------------------------------------------------
Great start! I've got several style nits, and some thoughts about extending beyond supersets. - How do you plan to test this? Have you tried just restarting a checkpointed slave process with new resources/attributes? I think you would run into a "Incompatible slave info detected" error. - We should start a conversation on the JIRA about how people would want to use this, what protobuf/cli/etc. API to define, and what to do about updates that are not supersets. src/common/attributes.cpp <https://reviews.apache.org/r/25111/#comment90329> s/Attributes &/Attributes& /g src/common/attributes.cpp <https://reviews.apache.org/r/25111/#comment90332> In your code, this.isSuperset(that) returns false if any of this's attributes are not present in that. It therefore returns true only if all of this's attributes exist in that, meaning this is a __subset__ of that. According to the comment, isSuperset should return true "if this is a superset of that". Perhaps this is supposed to be the implementation of isSubset? Maybe we should rename the function to read "this.isSupersetOf(that)", to disambiguate from the possibility that "this.isSuperset(that)" might mean "for object this, is that a superset of it?" src/common/attributes.cpp <https://reviews.apache.org/r/25111/#comment90330> isNone()? src/master/master.hpp <https://reviews.apache.org/r/25111/#comment90335> I'd like to discuss the implications of this with you. I wonder if there's some way we can enforce this, perhaps by wrapping the SlaveInfo with a watcher/bodyguard that notifies the registrar if the SlaveInfo changes. src/master/master.hpp <https://reviews.apache.org/r/25111/#comment90342> s/SlaveInfo &/SlaveInfo& / Move '{' to next line (to match the rest of the file) src/master/master.hpp <https://reviews.apache.org/r/25111/#comment90345> Please precede with a blank line src/master/master.hpp <https://reviews.apache.org/r/25111/#comment90351> Is this comment needed here? Looks like it was copied from ReadmitSlave src/master/master.hpp <https://reviews.apache.org/r/25111/#comment90347> Please end the comment with punctuation ('.'). src/master/master.hpp <https://reviews.apache.org/r/25111/#comment90348> s/Registry::Slave */Registry::Slave* / src/master/master.hpp <https://reviews.apache.org/r/25111/#comment90346> Precede with a blank line src/master/master.hpp <https://reviews.apache.org/r/25111/#comment90355> What do you mean by "push a slave"? Update its SlaveInfo? src/master/master.hpp <https://reviews.apache.org/r/25111/#comment90344> Add this line back in. We use double-blank lines between classes/elements at the top-level scope. src/master/master.cpp <https://reviews.apache.org/r/25111/#comment90340> s/attribuest/attributes/ src/master/master.cpp <https://reviews.apache.org/r/25111/#comment90341> Should probably handle the Future returned by registrar->apply(), in case it fails/discards, and/or if you want to wait for successful completion before performing any further actions. src/master/master.cpp <https://reviews.apache.org/r/25111/#comment90360> Probably not often. The slave would have to send two ReregisterSlaveMessages in the time it takes for the registrar update to occur. This could happen if a) The slave restarted quickly; or b) the registrar update takes too long, perhaps due to networking or disk errors. src/slave/slave.hpp <https://reviews.apache.org/r/25111/#comment90361> We shy away from abbreviations. How about 'newAttributes' and 'newResources' instead of 'attrs' and 'res'? (I realize 'attributes' and 'resources' are already taken) src/slave/slave.cpp <https://reviews.apache.org/r/25111/#comment90368> Who's going to call this? How about installing a protobuf handler for ReconfigureSlaveMessage or UpdateSlaveInfoMessage or something? If we have that just take an entire SlaveInfo, we'll have to check for hostname/port/SlaveID/checkpoint changes and disallow them for now, unless we do a full unregister/shutdown. src/slave/slave.cpp <https://reviews.apache.org/r/25111/#comment90336> Once you get it working for the superset, I'd like to discuss how we can relax this restriction. It's perfectly reasonable for us to rescind existing offers, and losses of attributes/resources may be acceptable if no current tasks are using them. But it could get into some fun race conditions if frameworks are already trying to launch tasks on a slave while it is reconfiguring. src/slave/slave.cpp <https://reviews.apache.org/r/25111/#comment90362> Make this a VLOG(1) message instead of INFO src/slave/slave.cpp <https://reviews.apache.org/r/25111/#comment90366> How about reversing the logic to !(newResources >= resources) so it reads more like the error message? src/slave/slave.cpp <https://reviews.apache.org/r/25111/#comment90364> s/attributes/resources/ src/slave/slave.cpp <https://reviews.apache.org/r/25111/#comment90367> Changing the slave's internal state to DISCONNECTED does not actually "unregister" it in the master. The master will continue to try to launch tasks on the slave, offer its resources up to the frameworks, and any other business as usual. This is purely internal state to notify the slave that it thinks it still needs to register. That approach is fine as long as we're sticking to strict supersets. If we allow subsets, the slave will have to stop accepting new tasks and tell the master to rescind its outstanding offers. Only then can the slave update its local state and reregister to update the master/registry. That would be more of an "unregister" approach. - Adam B On Aug. 27, 2014, 2:28 p.m., Patrick Reilly wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25111/ > ----------------------------------------------------------- > > (Updated Aug. 27, 2014, 2:28 p.m.) > > > Review request for mesos, Adam B and Benjamin Hindman. > > > Bugs: MESOS-1739 > https://issues.apache.org/jira/browse/MESOS-1739 > > > Repository: mesos-git > > > Description > ------- > > Add basic stub for dynamic slave attributes > > > Diffs > ----- > > src/common/attributes.hpp 0a043d5 > src/common/attributes.cpp aab114e > src/master/master.hpp c9f989a > src/master/master.cpp 2508b38 > src/slave/slave.hpp 9d4607e > src/slave/slave.cpp 5c76dd1 > > Diff: https://reviews.apache.org/r/25111/diff/ > > > Testing > ------- > > This is currently a work in progress, (WIP) > > > Thanks, > > Patrick Reilly > >
