> On March 11, 2017, 8:44 a.m., Jiang Yan Xu wrote: > > src/slave/slave.cpp > > Lines 1812-1823 (original), 1813-1822 (patched) > > <https://reviews.apache.org/r/55887/diff/10/?file=1661660#file1661660line1817> > > > > This comment to me is more misleading than helping. I don't understand > > why "Ideally we would perform the following check here". > > > > AFAICT the comment in its original place means: normally when you erase > > a task from `pending` you would check if the framework is fully cleaned up > > and if so remove it. However in the original location of this comment, it's > > removing tasks from `pending` **not for the sake of cleaning up** but > > rather because it's about to launch these tasks and store these in some > > other places, **so you can't remove the framework**. > > > > Here the meaning above is lost entirely: we could've checked the > > condition within the loop; there's no difference. > > > > We can check with @mpark (who added it) about how to improve the > > comment but for now let's keep it at the original location: in `__run` > > where you remove tasks from `pending` before you launch them.
I added this to `__run()` where `removePendingTask()` is called the first time, and removed references to this in `_run()`. > On March 11, 2017, 8:44 a.m., Jiang Yan Xu wrote: > > src/slave/slave.cpp > > Lines 1960 (patched) > > <https://reviews.apache.org/r/55887/diff/10/?file=1661660#file1661660line1980> > > > > Could we add a TODO to acknowledge the issue of code duplication and > > say we should consider refactoring it out? Added the TODO in `Slave::_run()`. - Anindya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55887/#review168693 ----------------------------------------------------------- On March 10, 2017, 10:15 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55887/ > ----------------------------------------------------------- > > (Updated March 10, 2017, 10:15 p.m.) > > > Review request for mesos, Adam B, Anand Mazumdar, Alexander Rojas, and Jiang > Yan Xu. > > > Bugs: MESOS-6953 > https://issues.apache.org/jira/browse/MESOS-6953 > > > Repository: mesos > > > Description > ------- > > Added support for action `run_tasks` on the agent's flag `acl`. Based on > the ACL configured for `run_tasks`, a task to be launched on the agent > can be (dis)allowed to launch on the agent. > If a task or task group cannot be launched due to failed authorization, > a `TASK_ERROR` Status Update shall be sent with a reason code of > `REASON_TASK_UNAUTHORIZED` or `REASON_TASK_GROUP_UNAUTHORIZED` as > applicable. > Note that in case of a task group, all tasks fail if any of the tasks > within the task group encounter the authorization error. > > > Diffs > ----- > > src/slave/slave.hpp 978edd6309dfbbde1058f9c44d5fac7083ff95fb > src/slave/slave.cpp 2308d5bf1fef5e1a6458a3bb742a16935a127929 > > > Diff: https://reviews.apache.org/r/55887/diff/10/ > > > Testing > ------- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
