----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55887/#review167654 -----------------------------------------------------------
src/slave/slave.hpp Lines 152-153 (original), 152-153 (patched) <https://reviews.apache.org/r/55887/#comment239563> If we are keeping the original semantics of `_run`, do we still need to change the code here and the tests to capture `__run` instead of `_run` for mocking? The mocking is just used to insert task killings when the task is not with the agent actor. src/slave/slave.hpp Lines 340 (patched) <https://reviews.apache.org/r/55887/#comment239564> If we don't make the change above, then here we have `__run` and `___run`. src/slave/slave.cpp Lines 1799-1808 (original), 1795-1804 (patched) <https://reviews.apache.org/r/55887/#comment239568> In `run` we have added these tasks to `framework->pending`, we should remove them here? src/slave/slave.cpp Lines 1809-1818 (original), 1805-1814 (patched) <https://reviews.apache.org/r/55887/#comment239571> We could probably move this comment to `__run` rather than referring back from there which is a bit strange. Plus rationale for additional explaination about "we need 'framework' to stay valid for the rest of this function" applies there more than here? src/slave/slave.cpp Lines 1830-1840 (original), 1818-1835 (patched) <https://reviews.apache.org/r/55887/#comment239565> This results in a behavior change: if tasks are killed during unscheduling of directories, we now send TASK_DROPPED instead of TASK_KILLED. I think this is a better behavior because this is race anyways and the TASK_DROPPED here better reveals the error condition on the host. Nevertheless we could check with the @anand and @vinod who last changed this. src/slave/slave.cpp Line 1858 (original), 1946 (patched) <https://reviews.apache.org/r/55887/#comment239569> Remove from `framework->pending`? src/slave/slave.cpp Lines 1960 (patched) <https://reviews.apache.org/r/55887/#comment239567> If we move the referred comment from `_run` to `__run` we probably don't need to change these comments for now but leave them as is: ``` // Refer to the comment after 'framework->pending.erase' above // for why we need this. ``` - Jiang Yan Xu On March 1, 2017, 8:15 a.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55887/ > ----------------------------------------------------------- > > (Updated March 1, 2017, 8:15 a.m.) > > > Review request for mesos, Adam B, 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 3b0aea4e3e9a17501077beccbccaab4abbe11af2 > src/slave/slave.cpp 892ce1938ac695e7913aa9139536d0787f3f5ea7 > src/tests/master_slave_reconciliation_tests.cpp > 1c7a3d686e2f924ad14c75fcab2ccafaab6d7b81 > src/tests/mock_slave.hpp 1acb961b642e1e0b4677db7b8fc6150d480eb751 > src/tests/mock_slave.cpp 50c04bff9e6f0f202af4c07b4036e021d3833ebf > src/tests/slave_tests.cpp 3731c7607d5e49f3000c4863b1999851fac45705 > > > Diff: https://reviews.apache.org/r/55887/diff/6/ > > > Testing > ------- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
