----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22367/#review46247 -----------------------------------------------------------
Looks great! The one thing I'm wondering is whether we want to expose 'unregister' as a method in the slave. Would we ever want to call 'unregister' and stay running? If not, it may make sense to just always send the unregister message when we self-terminate (when (!from) in 'shutdown'). src/master/master.cpp <https://reviews.apache.org/r/22367/#comment81558> How about storing the slave in the if condition so that we don't have to look it up over and over? I would suggest the following log message so that we can identify the PID mismatch in the logs: Slave* slave = getSlave(slaveId); if (slave != NULL) { if (from != slave->pid) { LOG(ERROR) << "Ignoring unregister slave message from " << from << " because it is not the slave " << slave->pid; } removeSlave(slave); } Notice no period at the end of the log message. :) src/slave/slave.cpp <https://reviews.apache.org/r/22367/#comment81559> How about: LOG(INFO) << message << "; sending unregister message to the master"; As is done inside the shutdown() logic when we self-terminate. - Ben Mahler On June 19, 2014, 3:24 p.m., Alexandra Sava wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22367/ > ----------------------------------------------------------- > > (Updated June 19, 2014, 3:24 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-544 > https://issues.apache.org/jira/browse/MESOS-544 > > > Repository: mesos-git > > > Description > ------- > > Second phase: Mesos-slave support for "node drain" > * create a method that sends UnregisterRequestMessage to the master > * dispatch that method in the signal handler, which executes when SIGUSR1 is > issued > > ** This review relies on https://reviews.apache.org/r/21379/ > > > Diffs > ----- > > src/master/master.hpp 2844446e2674df6e11f4c2915ed324fcc103532c > src/master/master.cpp 888657dd4bc50085882382908e3c48ccb857c621 > src/slave/slave.hpp 605ee4a9cebb3ace4cc6e9387cae1b08b2be8c6c > src/slave/slave.cpp ed3483ff1762d93837f328d1e647c490cf9e14c4 > > Diff: https://reviews.apache.org/r/22367/diff/ > > > Testing > ------- > > > Thanks, > > Alexandra Sava > >
