mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anindya Sinha <anindya_si...@apple.com>
Subject Re: Review Request 55887: Check task user before allowing a task to be launched on the agent.
Date Sun, 12 Mar 2017 08:05:09 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message