----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51477/#review147990 -----------------------------------------------------------
src/slave/slave.hpp (lines 327 - 329) <https://reviews.apache.org/r/51477/#comment215309> kill this comment. src/slave/slave.hpp (line 845) <https://reviews.apache.org/r/51477/#comment215310> we typically use 'get' instead of 'find' for these cases. src/slave/slave.cpp (line 1615) <https://reviews.apache.org/r/51477/#comment215312> couldn't this have been? ``` for (auto it = tasks.begin(); it != tasks.end(); ++it) ``` src/slave/slave.cpp (line 1709) <https://reviews.apache.org/r/51477/#comment215320> you say "other" tasks but you send TASK_KILLED for "all" tasks in the group. can you remind me why we can't send TASK_KILLED for the killed task in `killTask()` right away and send TASK_KILLED for the rest of the tasks in the group here, similar to what we do in `Master::_accept()`? src/slave/slave.cpp (line 1710) <https://reviews.apache.org/r/51477/#comment215321> if you follow `Master::_accept()` this should be a hashset of TaskIDs. src/slave/slave.cpp (line 1750) <https://reviews.apache.org/r/51477/#comment215322> back ticks instead of quotes. src/slave/slave.cpp (line 1788) <https://reviews.apache.org/r/51477/#comment215323> space before "directories" src/slave/slave.cpp (line 1840) <https://reviews.apache.org/r/51477/#comment215324> s/task/task or task group/ src/slave/slave.cpp (line 1898) <https://reviews.apache.org/r/51477/#comment215326> back ticks. src/slave/slave.cpp (line 2055) <https://reviews.apache.org/r/51477/#comment215332> extra space after CHECK src/slave/slave.cpp (line 2058) <https://reviews.apache.org/r/51477/#comment215331> extra space after CHECK src/slave/slave.cpp (line 2104) <https://reviews.apache.org/r/51477/#comment215330> i think you need to also insert a space after each task group? it's probably best to store taskgroups as a vector of vectors and call stringify. src/slave/slave.cpp (line 2171) <https://reviews.apache.org/r/51477/#comment215335> but `killTask` is not sending a TASK_KILLED update anymore? src/slave/slave.cpp (line 2198) <https://reviews.apache.org/r/51477/#comment215336> CHECK_SOME(taskGroups); ? src/slave/slave.cpp (lines 2321 - 2323) <https://reviews.apache.org/r/51477/#comment215338> s/bigger// can you remind me why we don't send TASK_KILLED here? AFAICT `_run()` has information about the task group based on the argument passed to it, even if `framework->pending` doesn't itself have that info. if there is no good reason, lets make this consistent with how we do it in the master. src/slave/slave.cpp <https://reviews.apache.org/r/51477/#comment215339> can you add a comment here for why we don't remove the framework here for posterity? src/slave/slave.cpp (line 2360) <https://reviews.apache.org/r/51477/#comment215340> s/other tasks/tasks in the group/ src/slave/slave.cpp (line 2451) <https://reviews.apache.org/r/51477/#comment215341> s/task/task or task group/ src/slave/slave.cpp (line 2455) <https://reviews.apache.org/r/51477/#comment215342> why the return? src/slave/slave.cpp (line 3063) <https://reviews.apache.org/r/51477/#comment215343> tasks or task groups. src/slave/slave.cpp (lines 3102 - 3105) <https://reviews.apache.org/r/51477/#comment215345> you couldn't use the default assignment operator for LinkedHashMap? src/slave/slave.cpp (lines 3116 - 3138) <https://reviews.apache.org/r/51477/#comment215347> it's unfortunate that you have to make 2 duplicate containerizer->update calls here. is there any reason why `__run` work with both tasks and taskGroups arguments set? src/slave/slave.cpp (lines 3327 - 3367) <https://reviews.apache.org/r/51477/#comment215348> ditto. see comments above. src/slave/slave.cpp (lines 6677 - 6687) <https://reviews.apache.org/r/51477/#comment215349> looks fancy, why not just do: ``` foreach (const TaskGroup& taskGroup, queuedTaskGroups) { foreach (const TaskInfo& taskInfo, taskGroup.tasks()) { if (taskInfo.task_id() == taskId) { return taskGroup; } } } return None(); ``` - Vinod Kone On Sept. 6, 2016, 9:25 p.m., Anand Mazumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51477/ > ----------------------------------------------------------- > > (Updated Sept. 6, 2016, 9:25 p.m.) > > > Review request for mesos and Vinod Kone. > > > Bugs: MESOS-6076 > https://issues.apache.org/jira/browse/MESOS-6076 > > > Repository: mesos > > > Description > ------- > > This changes implements the `runTaskGroup()` handler on the > agent ensuring that task group is sent atomically to the executor > via the `LAUNCH_GROUP` event. It also refactors `runTask()`/`_runTask()` > to go through a common handler function. Also, it ensures that all > tasks in `framework->pending`/`queuedTasks` that are killed before > running the task group result in all the tasks being killed. > > Review: https://reviews.apache.org/r/51477/ > > > Diffs > ----- > > src/slave/slave.hpp 4add4c0180ea56039e0d5009bad4d9346128bde6 > src/slave/slave.cpp 11664779ed78c0a5913598bb7dd1bb0e793d6b93 > > Diff: https://reviews.apache.org/r/51477/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Anand Mazumdar > >
