mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vinod Kone <vinodk...@gmail.com>
Subject Re: Review Request 51477: Implemented `RunTaskGroupMessage` handler on the agent.
Date Wed, 07 Sep 2016 10:26:38 GMT

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




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

    kill this comment.



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

    we typically use 'get' instead of 'find' for these cases.



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

    couldn't this have been?
    
    ```
    for (auto it = tasks.begin(); it != tasks.end(); ++it)
    ```



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

    you say "other" tasks but you send TASK_KILLED for "all" tasks in the group. can you remind
me why we can't send TASK_KILLED for the killed task in `killTask()` right away and send TASK_KILLED
for the rest of the tasks in the group here, similar to what we do in `Master::_accept()`?



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

    if you follow `Master::_accept()` this should be a hashset of TaskIDs.



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

    back ticks instead of quotes.



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

    space before "directories"



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

    s/task/task or task group/



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

    back ticks.



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

    extra space after CHECK



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

    extra space after CHECK



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

    i think you need to also insert a space after each task group? it's probably best to store
taskgroups as a vector of vectors and call stringify.



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

    but `killTask` is not sending a TASK_KILLED update anymore?



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

    CHECK_SOME(taskGroups); ?



src/slave/slave.cpp (lines 2321 - 2323)
<https://reviews.apache.org/r/51477/#comment215338>

    s/bigger//
    
    can you remind me why we don't send TASK_KILLED here? AFAICT `_run()` has information
about the task group based on the argument passed to it, even if `framework->pending` doesn't
itself have that info. if there is no good reason, lets make this consistent with how we do
it in the master.



src/slave/slave.cpp 
<https://reviews.apache.org/r/51477/#comment215339>

    can you add a comment here for why we don't remove the framework here for posterity?



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

    s/other tasks/tasks in the group/



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

    s/task/task or task group/



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

    why the return?



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

    tasks or task groups.



src/slave/slave.cpp (lines 3102 - 3105)
<https://reviews.apache.org/r/51477/#comment215345>

    you couldn't use the default assignment operator for LinkedHashMap?



src/slave/slave.cpp (lines 3116 - 3138)
<https://reviews.apache.org/r/51477/#comment215347>

    it's unfortunate that you have to make 2 duplicate containerizer->update calls here.
is there any reason why `__run` work with both tasks and taskGroups arguments set?



src/slave/slave.cpp (lines 3327 - 3367)
<https://reviews.apache.org/r/51477/#comment215348>

    ditto. see comments above.



src/slave/slave.cpp (lines 6677 - 6687)
<https://reviews.apache.org/r/51477/#comment215349>

    looks fancy, why not just do:
    
    ```
    foreach (const TaskGroup& taskGroup, queuedTaskGroups) {
      foreach (const TaskInfo& taskInfo, taskGroup.tasks()) {
        if (taskInfo.task_id() == taskId) {
          return taskGroup;  
        }
      }
    }
    
    return None();
    
    ```


- Vinod Kone


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