----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19795/#review39335 -----------------------------------------------------------
src/slave/slave.hpp <https://reviews.apache.org/r/19795/#comment71686> I think this is leaking too much implementation detail? How about just saying // Schedules __runTask() once the executor is launched. src/slave/slave.hpp <https://reviews.apache.org/r/19795/#comment71689> How about: Is called when the executor info is ready. src/slave/slave.hpp <https://reviews.apache.org/r/19795/#comment71723> It just seems a bit confusing to have this static method in this struct when we have a "id" variable. How about pulling this up to Master class? src/slave/slave.hpp <https://reviews.apache.org/r/19795/#comment71690> s/not be/not/ src/slave/slave.hpp <https://reviews.apache.org/r/19795/#comment71691> s/is ready/will be ready/ src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71692> not in alphabetical order. src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71694> Instead of constructing and passing the message why not do the message construction in _doReliableRegistration()? That way you would avoid any nasty races. src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71721> s/Driver/driver/ src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71725> Pull this inside the if below. src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71727> put .onAny on the new line. executor->info .onAny(defer(..., ...)); src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71746> Do we need this future passed through considering we have the executorInfo member variable? src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71734> log the executor id. src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71732> Why did you move this down to here? If a framework goes to TERMINATING before __runTask() gets called, the framework would never be removed. This should be below line #991. src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71735> ditto. formatting. src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71747> ditto. Do we need this future passed through considering we have the executorInfo member variable? src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71736> log the task. src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71738> I don't think the slave can be in RECOVERING state here? Also, do we want to proceed if the slave is TERMINATING? src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71745> CHECK_NOTNULL(executor); src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71739> ditto. formatting. src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71748> ditto. Do we need this future passed through considering we have the executorInfo member variable? src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71741> I don't think slave can be in RECOVERING state here. src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71742> log the executor. src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71743> CHECK_NOTNULL(executor); src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71756> Log that we are shutting this down. src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71758> formatting. src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71761> ditto. do we need to pass the future? src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71760> Can this be in any other state than RECOVERING? src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71774> Why is this returning a Future? Is this for future proofing (no pun intended). src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71762> This seems to be a weird failure message. src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71775> quotes around executor ids. src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71776> No need to extract this into a method (see my comments later). src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71777> this formatting is different from others. src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71769> Do you want to address this TODO now? I'm afraid that if you only make ExecutorInfo a future when we revisit this TODO we have to go through the same pain of refactoring? src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71770> In fact I think even the directory should be handled by the containerizer? src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71768> Do this check at the beginning of this method. src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71772> why the change to .then? src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71766> I don't think you need this. It is not possible that you are here if "state.info.isNone()" because that means state.runs.empty() and state.latest.isNone(). IOW, it's not possible that you have non-zero runs but not have executor info. Does that make sense? To make this precise, in fact you can add state.info.isNone() to the if check at the top and do a CHECK(state.info.isSome()) here. src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71767> All you are doing here during recovery is to setup the resources. Just do it directly instead of calling launched(). src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71763> hmm. we should make this optional instead of setting it to empty. src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment71765> CHECK_NOTNULL(slave) - Vinod Kone On April 2, 2014, 1:45 a.m., Niklas Nielsen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19795/ > ----------------------------------------------------------- > > (Updated April 2, 2014, 1:45 a.m.) > > > Review request for mesos, Ian Downes and Vinod Kone. > > > Bugs: MESOS-922 > https://issues.apache.org/jira/browse/MESOS-922 > > > Repository: mesos-git > > > Description > ------- > > This is the 2nd part of the task-info patch split > (https://reviews.apache.org/r/18403/) and changes Executor::info to an > executor info future. > This is motivated by delegating executor info creation/choice to the > containerizer to address new container/executor scenarios > (https://issues.apache.org/jira/browse/MESOS-922). > > This patch use the new Executor::info and introduces new continuations to > deal with launching containers i.e. executor infos are to be determined. > > > Diffs > ----- > > src/slave/http.cpp 594032d > src/slave/slave.hpp 15e23ce > src/slave/slave.cpp a356f5f > > Diff: https://reviews.apache.org/r/19795/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Niklas Nielsen > >
