----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11694/#review21559 -----------------------------------------------------------
Ship it! Really really really nice work Vinod. This is super clean and IMHO easy to reason about. Nice to have some clean semantics re: task lifecycle. src/slave/slave.hpp <https://reviews.apache.org/r/11694/#comment44599> Food for thought: s/removeTask/completeTask/ src/slave/slave.hpp <https://reviews.apache.org/r/11694/#comment44598> Ha, BenM and I had a conversation today about the use of the word 'pending' in Hadoop (i.e., does pendingMaps/Reduces() mean maps/reduces that we still need to launch or maps/reduces that have been launched but haven't completed yet?). I think that the same thing applies here. Maybe s/pendingTasks/unfinishedTasks/ or better incompleteTasks (since the ring buffer is "completedTasks")? IMHO it's easier to interpret 'incompleteTasks' as implying _both_ tasks that are queued (what some might consider "pending") and those that are already launched. src/slave/slave.cpp <https://reviews.apache.org/r/11694/#comment44597> With this change it seems like "Cannot kill task" is a bit dishonest, since we are removing the task from queue and sending a TASK_KILLED status update. src/slave/slave.cpp <https://reviews.apache.org/r/11694/#comment44600> I'd prefer if we were as explicit as possible: "... removes the task from Executor::queuedTasks so that if the executor registers ..." src/slave/slave.cpp <https://reviews.apache.org/r/11694/#comment44601> Doing executor->completeTask(taskId) kind of has a nice ring to it, at least here. ;) It also nicely captures that the task still sticks around in an immutable state for showing in the webui. src/slave/slave.cpp <https://reviews.apache.org/r/11694/#comment44602> Haha, "for e.g:". ;) src/slave/slave.cpp <https://reviews.apache.org/r/11694/#comment44603> :-) - Benjamin Hindman On June 6, 2013, 8:39 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11694/ > ----------------------------------------------------------- > > (Updated June 6, 2013, 8:39 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/slave/slave.hpp 26dc96e5f2fdc0711fc49a9ea80b7f037bf93c29 > src/slave/slave.cpp 8ce1646f2f804bc8dc20506d11078191f0274654 > > Diff: https://reviews.apache.org/r/11694/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
