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


Niq, see my detailed comments. cc @tnachen, @vinodkone, @idownes, @bmahler


src/slave/slave.cpp
<https://reviews.apache.org/r/34720/#comment140150>

    First of all, we don't need to check if 'executor' is NULL or not here. If 
you looked at the callers, it's guaranteed that 'executor' will be non-NULL. So 
you need a CHECK here.
    
    Also, I thought about what the correct logic should be in this funtion (see 
below). Let me know your thoughts. (we have too many tech debts in this 
function, so we better fix this asap, instead of waiting further).
    
    ```
    TaskState state;
    TaskStatus::Reason reason;
    string message;
    
    // Determine the state and reason for the status update.
    if (termination.isReady() && termination.get().has_reason()) {
      // This is the case where the resource limit has been
      // reached and the container destroyed itself.
      state = TASK_FAILED;
      reason = termination.get().reason();
    } else if (executor->reason.isSome()) {
      // This is the case where the slave initiates the
      // termination of the container.
      
      // NOTE: For command executors, previously we send TASK_FAILED
      // and REASON_COMMAND_EXECUTOR_FAILED when the command executor
      // failed to launch (e.g., invalid executor path).
      if (executor->isCommandExecutor() &&
          executor->reason.get() == REASON_EXECUTOR_REGISTRATION_TIMEOUT) {
        state = TASK_FAILED;
        reason = REASON_COMMAND_EXECUTOR_FAILED;
      } else {
        state = TASK_LOST;
        reason = executor->reason.get();
      }
    } else {
      state = TASK_LOST;
      reason = TaskStatus::REASON_EXECUTOR_TERMINATED
    }
    
    // Determine the message for the status update.
    // TODO(nnielsen): Consider adding a message from the slave
    // as well, and concatenating the messages.
    if (termination.isReady()) {
      message = termination.get().message();
    } else {
      // This is case where the containerizer fails
      // to destroy the container.
      message = "Abnormal executor termination"; 
    }
    
    statusUpdate(...);
    ```


- Jie Yu


On June 11, 2015, 10:58 p.m., Niklas Nielsen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34720/
> -----------------------------------------------------------
> 
> (Updated June 11, 2015, 10:58 p.m.)
> 
> 
> Review request for mesos, Bartek Plotka, Jie Yu, Joris Van Remoortere, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-2653
>     https://issues.apache.org/jira/browse/MESOS-2653
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp 0df1b55791963fb4159b7ea5318d09dde4f7d8c7 
>   src/slave/slave.cpp b523c2fce50e56f4f94d55a9488f49c53452e4d4 
> 
> Diff: https://reviews.apache.org/r/34720/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Niklas Nielsen
> 
>

Reply via email to