----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19795/#review39699 -----------------------------------------------------------
I had these comments from revision 3 which I neglected to publish. They may be out of date for revision 4. src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment72286> I believe Vinod is saying the future argument to __runTask is the same as the member variable executor->info. You can do any error checking on the member variable, after you look up the Executor. src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment72316> +1. In this review nothing checks the return from executorLaunched so these failures won't be noticed and the errors won't be logged! Can you revert this part and defer (hah) it to later? src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment72290> s/future to chosen/a future to the chosen/ src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment72289> s/match/matches/ src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment72317> You don't know when this continuation runs after executorTerminated so you need to test framework == NULL and executor == NULL? src/slave/slave.cpp <https://reviews.apache.org/r/19795/#comment72318> +1 what happens if the launch fails? is it only detected at executor registration timeout? - Ian Downes On April 4, 2014, 12:22 a.m., Niklas Nielsen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19795/ > ----------------------------------------------------------- > > (Updated April 4, 2014, 12:22 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 > >
