mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vinod Kone <vinodk...@apache.org>
Subject Re: Review Request 65504: Made master set `launch_executor` in the RunTask(Group)Message.
Date Thu, 08 Feb 2018 20:03:54 GMT

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




src/master/master.cpp
Line 3832 (original), 3832 (patched)
<https://reviews.apache.org/r/65504/#comment277210>

    Can you add a comment here that for command tasks, even though an executor is launched
by the agent master is oblivious to that fact? Alternatively, this should return true for
command tasks.
    
    My proposal:
    
    ```
    bool islaunchExecutor(
       const ExecutorID& executorId,
       Framework* framework,
       Slave* slave) {
    
       CHECK_NOTNULL(framework);
       CHECK_NOTNULL(slave);
       
       if (!slave->hasExecutor(framework->id(), executorId())) {
          CHECK(!framework->hasExecutor(slave->id, executorId()))
            << "Executor '" << executorId
            << "' known to the framework " << *framework
            << " but unknown to the agent " << *slave;
    
          return true;
        }
      }
      
      return false;
    }
    ```



src/master/master.cpp
Lines 4922 (patched)
<https://reviews.apache.org/r/65504/#comment277211>

    ```
    if (launchExecutor && task.has_executor()) {
      addExecutor(task.executor(), framework, slave);
      consumed += task.executor().resources();
    }
    ```



src/master/master.cpp
Lines 4992 (patched)
<https://reviews.apache.org/r/65504/#comment277212>

    s/message.launch_executor()/launchExecutor/



src/master/master.cpp
Lines 5154-5165 (patched)
<https://reviews.apache.org/r/65504/#comment277215>

    ```
    bool launchExecutor = isLaunchExecutor(executor.executor_id(), framework, slave);
    if (launchExecutor) {
      addExecutor(executor, framework, slave);
      executorResources = executor.resources();
      totalResources += executorResources;
    }
    
    ```



src/master/master.cpp
Lines 5185-5186 (patched)
<https://reviews.apache.org/r/65504/#comment277218>

    this doesn't seem correct?
    
    shouldn't this be
    
    ```
    CHECK(_offeredResources.contains(taskResources))
         << _offeredResources << " does not contain " << taskResources;
     
    _offeredResources -= taskResources;
    
    ```
    
    also, you need to do another check for executor resources before the `foreach` loop
    
    ```
    CHECK(_offeredResources.contains(executorResources))
         << _offeredResources << " does not contain " << executorResources;
     
    _offeredResources -= executorResources;
    
    ```



src/master/master.cpp
Line 5159 (original), 5201 (patched)
<https://reviews.apache.org/r/65504/#comment277222>

    if we do the changes above maybe we can just have `taskGroupResources` variable instead
of `totalResources` and use it here?



src/master/master.cpp
Lines 5203 (patched)
<https://reviews.apache.org/r/65504/#comment277219>

    s/message.launch_executor()/executor/


- Vinod Kone


On Feb. 5, 2018, 2:49 a.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65504/
> -----------------------------------------------------------
> 
> (Updated Feb. 5, 2018, 2:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Vinod Kone.
> 
> 
> Bugs: MESOS-1720
>     https://issues.apache.org/jira/browse/MESOS-1720
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> By setting a new field `launch_executor` in the RunTask(Group)Message,
> the master is able to control the executor creation on the agent.
> 
> Also refactored `addTask()` logic. Added two new functions:
> `isTaskLaunchExecutor()` checks if a task needs to launch executor;
> `addExecutor()` adds executor to the framework and slave.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2 
>   src/master/master.cpp cc2685a6bc14103c639ce776cf1c912361e93381 
> 
> 
> Diff: https://reviews.apache.org/r/65504/diff/1/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


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