mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Guangya Liu <gyliu...@gmail.com>
Subject Re: Review Request 51477: Implemented `RunTaskGroupMessage` handler on the agent.
Date Wed, 07 Sep 2016 10:17:48 GMT

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




src/slave/slave.hpp (line 325)
<https://reviews.apache.org/r/51477/#comment215316>

    s/tasks/tasks or task groups



src/slave/slave.hpp (line 326)
<https://reviews.apache.org/r/51477/#comment215317>

    s/RunTaskMessages/RunTaskMessages or LaunchGroup



src/slave/slave.hpp (lines 327 - 329)
<https://reviews.apache.org/r/51477/#comment215315>

    kill this?



src/slave/slave.hpp (line 845)
<https://reviews.apache.org/r/51477/#comment215318>

    How about 
    s/findQueuedTaskGroup/getQueuedTaskGroup



src/slave/slave.hpp (line 1027)
<https://reviews.apache.org/r/51477/#comment215319>

    How about const?



src/slave/slave.cpp (line 1709)
<https://reviews.apache.org/r/51477/#comment215325>

    s/for/to



src/slave/slave.cpp (line 1746)
<https://reviews.apache.org/r/51477/#comment215327>

    What about enhance the reason as `Task killed before it was launched due to one task killed
in the task group`?



src/slave/slave.cpp (lines 1823 - 1845)
<https://reviews.apache.org/r/51477/#comment215328>

    I think the reason is not correct for other tasks in the task group.
    
    What about sending out TASK_LOST reason separately as following logic:
    
    if checkpoint failure:
      task lost with reason as checkpoint failure
      
    if kill:
      foreach task but not the checkpoint failure task:
         task lost with reason as one task fail cause whole task groupp fail



src/slave/slave.cpp (lines 1863 - 1884)
<https://reviews.apache.org/r/51477/#comment215329>

    ditto as above for updating `reasons` separately



src/slave/slave.cpp (lines 1980 - 1981)
<https://reviews.apache.org/r/51477/#comment215333>

    What about adding `executor state` to the log message?



src/slave/slave.cpp (lines 2002 - 2003)
<https://reviews.apache.org/r/51477/#comment215334>

    ditto



src/slave/slave.cpp (line 2370)
<https://reviews.apache.org/r/51477/#comment215337>

    Just a question here, we are killing task in task group, and we can even say here is killing
a task group, but here the status is still `TASK_KILLED`, do we need to introduce a new `TASKGROUP_KILLED`
for this?
    
    Ditto for other places.



src/slave/slave.cpp (lines 3099 - 3114)
<https://reviews.apache.org/r/51477/#comment215346>

    Can you please show more detail and update the comments here for which case will cause
the `executor->queuedTasks` and `executor->queuedTaskGroups` have same taskId?


- Guangya Liu


On 九月 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 九月 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