----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55887/#review168693 -----------------------------------------------------------
Fix it, then Ship it! Overall LGTM, just some nits. src/slave/slave.hpp Lines 1112 (patched) <https://reviews.apache.org/r/55887/#comment240979> Yeah you are right, `executorId` doesn't necessarily come from `task.executor()`. But for symmetry, use `ExecutorInfo` instead? ``` bool removePendingTask( const TaskInfo& task, const ExecutorInfo& executorInfo); ``` src/slave/slave.cpp Lines 1808 (patched) <https://reviews.apache.org/r/55887/#comment240980> s/list of// Nit-picking perhaps but we don't have to say "list" here and it's a map. :) src/slave/slave.cpp Line 1810 (original), 1811 (patched) <https://reviews.apache.org/r/55887/#comment240969> I am not sure if we need to check the result of `removePendingTask` or add this log line. It'll happen if it's killed and we have a log message for that in `Slave::killTask` so the information here is not of much significance? src/slave/slave.cpp Lines 1812-1823 (original), 1813-1822 (patched) <https://reviews.apache.org/r/55887/#comment240970> 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. src/slave/slave.cpp Lines 1826-1827 (patched) <https://reviews.apache.org/r/55887/#comment240981> Per the reason above, we probably don't need to keep repeating this reference in `_run` (as the comment should be moved to `__run`). src/slave/slave.cpp Lines 1840 (patched) <https://reviews.apache.org/r/55887/#comment240972> Althought it's safe in this case because we have checked, we are making an effort to switch to `at()` in the codebase for read access so here let's use: ``` framework->pending.at(executorId).contains(_task.task_id()) ``` src/slave/slave.cpp Lines 1852 (patched) <https://reviews.apache.org/r/55887/#comment240983> I feel the name `removePendingTask` is self documenting enough? src/slave/slave.cpp Lines 1854 (patched) <https://reviews.apache.org/r/55887/#comment240982> Ditto, no need to check or log here. src/slave/slave.cpp Lines 1898 (patched) <https://reviews.apache.org/r/55887/#comment240984> Ditto, the method name is self-documenting. src/slave/slave.cpp Lines 1900 (patched) <https://reviews.apache.org/r/55887/#comment240985> Ditto about log line. src/slave/slave.cpp Lines 1960 (patched) <https://reviews.apache.org/r/55887/#comment240986> Could we add a TODO to acknowledge the issue of code duplication and say we should consider refactoring it out? src/slave/slave.cpp Lines 2067-2070 (patched) <https://reviews.apache.org/r/55887/#comment240977> For ExecutorInfo, we should look at `executorInfo` and not `_task.executor()` because it might not be set? src/slave/slave.cpp Lines 2088 (patched) <https://reviews.apache.org/r/55887/#comment240987> s/Failing/Ignoring running/? Failing may be implying `TASK_FAILED` when it's `TASK_ERROR`? src/slave/slave.cpp Lines 6883 (patched) <https://reviews.apache.org/r/55887/#comment240988> I just realized it might be better to use an affirmative statement... ``` // Return `true` if `task` was a pending task of this framework before the removal. ``` src/slave/slave.cpp Lines 6889 (patched) <https://reviews.apache.org/r/55887/#comment240978> Use `.at()`? Sorry my code example didn't update this. - Jiang Yan Xu On March 10, 2017, 2: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, 2: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 > >
