----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55887/#review166890 -----------------------------------------------------------
src/slave/slave.cpp (line 1706) <https://reviews.apache.org/r/55887/#comment238967> The code below doesn't do "reporting", move to where it's done? src/slave/slave.cpp (line 1717) <https://reviews.apache.org/r/55887/#comment238973> w.r.t the `unschedule` here. This looks problematic: 1. we are invoking some asynchronous work (captured by `unschedule`) without checking it's result but only to pass it along, then why not do it afterwards? 2. we are not cleaning up the work that is initiated above (as well as some internal state changes like `framework->pending`) if `authorizations` fails. We don't look at `unschedule` at all for that case. 3. semantically we should authorize the task before a whole bunch of work with side-effects are initiated. It should probably be the first thing we do after some simple checking. We should do the unscheduling after we have authorize the tasks. src/slave/slave.cpp (lines 1747 - 1748) <https://reviews.apache.org/r/55887/#comment238977> Move this to before we do it? At this point authorization is already done. src/slave/slave.cpp (line 1759) <https://reviews.apache.org/r/55887/#comment238982> There's 3rd state here, since we don't expect anyone to discard the future, we should ``` CHECK(!future.isDiscarded()); ``` before assuming `future.failure()` works. src/slave/slave.cpp (line 6258) <https://reviews.apache.org/r/55887/#comment238984> authorize is a transitive verb so we need s/authorized/authorized it/? src/slave/slave.cpp (line 6266) <https://reviews.apache.org/r/55887/#comment238985> In case it becomes an issue to construct `Framework` before `authorizeTask`, we actually only need `FrameworkInfo` here. - Jiang Yan Xu On Feb. 20, 2017, 11:20 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55887/ > ----------------------------------------------------------- > > (Updated Feb. 20, 2017, 11:20 p.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_FAILED` 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 7564e8d39530794131dbbc928fcbc59fb65ef471 > 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 16bb14b170d724bd8424778a76de28b0efccc6ed > > Diff: https://reviews.apache.org/r/55887/diff/ > > > Testing > ------- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
