mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anand Mazumdar <an...@apache.org>
Subject Re: Review Request 51477: Implemented `RunTaskGroupMessage` handler on the agent.
Date Tue, 06 Sep 2016 21:26:09 GMT


> On Sept. 1, 2016, 5:48 a.m., Qian Zhang wrote:
> > src/slave/slave.cpp, line 1882
> > <https://reviews.apache.org/r/51477/diff/2/?file=1487469#file1487469line1882>
> >
> >     I do not think we need this, because if we get here, `kill` must be still `false`.

Good catch! I added an explicit invariant check here.


> On Sept. 1, 2016, 5:48 a.m., Qian Zhang wrote:
> > src/slave/slave.cpp, line 1951
> > <https://reviews.apache.org/r/51477/diff/2/?file=1487469#file1487469line1951>
> >
> >     Not yours, but I think we do not need this blank line.

We might want to clean this up in a separate patch.


> On Sept. 1, 2016, 5:48 a.m., Qian Zhang wrote:
> > src/slave/slave.cpp, line 1754
> > <https://reviews.apache.org/r/51477/diff/2/?file=1487469#file1487469line1754>
> >
> >     I think we can `break` from here.

hmm, we can't `break` here. Let's say we had a task group containing tasks: T1, T2 & T3.
We put them in the pending queue in `_runTask()`.

A scheduler sends a `killTask()` for T2 before `__runTask()` is invoked. We remove T2 from
pending in `killTask()`.

When we are in `__runTask`, pending is T1 & T3. If we break in the loop just after `T2`,
we would leave behind `T3` still in `framework->pending`.


> On Sept. 1, 2016, 5:48 a.m., Qian Zhang wrote:
> > src/slave/slave.cpp, line 2542
> > <https://reviews.apache.org/r/51477/diff/2/?file=1487469#file1487469line2542>
> >
> >     Here we send a `KillTaskMessage` to executor, but the task to be killed may
be part of a task group, so that means it is executor's responsibility to kill the tasks in
the whole group, right? What if executor does not do it? Will we enforce it in agent?

We have never enforced this on the agent (historically for the single task case too)


- Anand


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51477/#review147401
-----------------------------------------------------------


On Sept. 6, 2016, 9:25 p.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51477/
> -----------------------------------------------------------
> 
> (Updated Sept. 6, 2016, 9:25 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6076
>     https://issues.apache.org/jira/browse/MESOS-6076
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This changes implements the `runTaskGroup()` handler on the
> agent ensuring that task group is sent atomically to the executor
> via the `LAUNCH_GROUP` event. It also refactors `runTask()`/`_runTask()`
> to go through a common handler function. Also, it ensures that all
> tasks in `framework->pending`/`queuedTasks` that are killed before
> running the task group result in all the tasks being killed.
> 
> Review: https://reviews.apache.org/r/51477/
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp 4add4c0180ea56039e0d5009bad4d9346128bde6 
>   src/slave/slave.cpp 11664779ed78c0a5913598bb7dd1bb0e793d6b93 
> 
> Diff: https://reviews.apache.org/r/51477/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>


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